From 2e15f510f02b851bccbf08e168f0403f88b1715b Mon Sep 17 00:00:00 2001 From: dfliess Date: Mon, 14 Sep 2026 13:05:01 +0200 Subject: [PATCH] fix: open alert and report links on metrics views without a time dimension The "open" page of alerts and reports always requested the time range summary of the metrics view, so on a metrics view without a time dimension it failed with "no time dimension specified". Fetch the summary only when the metrics view has a time dimension, skip the time range mapping otherwise, and do not open the time dimension details page without one. Co-Authored-By: Claude Opus 5 (1M context) --- ...dashboard-from-aggregation-request.spec.ts | 238 +++++++++++++++++- .../get-dashboard-from-aggregation-request.ts | 3 +- ...-dashboard-from-comparison-request.spec.ts | 130 ++++++++++ .../explore-mappers/map-to-explore.ts | 52 ++-- .../src/features/explore-mappers/types.ts | 3 +- .../src/features/explore-mappers/utils.ts | 5 +- 6 files changed, 401 insertions(+), 30 deletions(-) create mode 100644 web-common/src/features/explore-mappers/get-dashboard-from-comparison-request.spec.ts diff --git a/web-common/src/features/explore-mappers/get-dashboard-from-aggregation-request.spec.ts b/web-common/src/features/explore-mappers/get-dashboard-from-aggregation-request.spec.ts index 632faddc0d43..ed74fa98a891 100644 --- a/web-common/src/features/explore-mappers/get-dashboard-from-aggregation-request.spec.ts +++ b/web-common/src/features/explore-mappers/get-dashboard-from-aggregation-request.spec.ts @@ -15,6 +15,7 @@ import { AD_BIDS_EXPLORE_NAME, AD_BIDS_EXPLORE_WITH_3_MEASURES_DIMENSIONS, AD_BIDS_IMPRESSIONS_MEASURE, + AD_BIDS_METRICS_3_MEASURES_DIMENSIONS, AD_BIDS_METRICS_3_MEASURES_DIMENSIONS_WITH_TIME, AD_BIDS_METRICS_NAME, AD_BIDS_PUBLISHER_DIMENSION, @@ -29,12 +30,23 @@ import { import { waitUntil } from "@rilldata/web-common/lib/waitUtils.ts"; import { DashboardState_ActivePage } from "@rilldata/web-common/proto/gen/rill/ui/v1/dashboard_pb.ts"; import { + type V1ExploreSpec, type V1MetricsViewAggregationRequest, + type V1MetricsViewSpec, V1Operation, V1TimeGrain, + type V1TimeRangeSummary, } from "@rilldata/web-common/runtime-client"; import { RuntimeClient } from "@rilldata/web-common/runtime-client/v2"; -import { beforeEach, describe, expect, it } from "vitest"; +import { + afterEach, + beforeEach, + describe, + expect, + it, + type MockInstance, + vi, +} from "vitest"; describe("getDashboardFromAggregationRequest", () => { const mocks = DashboardFetchMocks.useDashboardFetchMocks(); @@ -383,6 +395,167 @@ describe("getDashboardFromAggregationRequest", () => { ]); }); + describe("metrics view without a time dimension", () => { + let fetchSpy: MockInstance; + + beforeEach(() => { + mocks.mockMetricsView(NO_TIME_SOURCE.metricsViewName, NO_TIME_METRICS); + mocks.mockMetricsExplore( + NO_TIME_SOURCE.exploreName, + NO_TIME_METRICS, + NO_TIME_EXPLORE, + ); + fetchSpy = vi.spyOn(globalThis, "fetch"); + }); + + afterEach(() => { + fetchSpy.mockRestore(); + }); + + // The runtime rejects a time range summary request for this metrics view. + function expectNoTimeRangeSummaryRequest() { + expect( + fetchSpy.mock.calls.filter(([input]) => + (input instanceof Request ? input.url : input.toString()).endsWith( + "/MetricsViewTimeRange", + ), + ), + ).toEqual([]); + } + + const where = createAndExpression([ + createInExpression(AD_BIDS_PUBLISHER_DIMENSION, ["Yahoo"]), + ]); + const TestCases: { + title: string; + aggregationRequest: V1MetricsViewAggregationRequest; + expectedNonPivotState: Partial; + expectedPivotState: Partial; + }[] = [ + { + title: "With a dimension, measure and filter", + aggregationRequest: { + dimensions: [{ name: AD_BIDS_DOMAIN_DIMENSION }], + measures: [{ name: AD_BIDS_BID_PRICE_MEASURE }], + sort: [{ desc: true, name: AD_BIDS_BID_PRICE_MEASURE }], + where, + }, + expectedNonPivotState: { + activePage: DashboardState_ActivePage.DIMENSION_TABLE, + allMeasuresVisible: false, + visibleMeasures: [AD_BIDS_BID_PRICE_MEASURE], + selectedDimensionName: AD_BIDS_DOMAIN_DIMENSION, + leaderboardSortByMeasureName: AD_BIDS_BID_PRICE_MEASURE, + whereFilter: where, + }, + expectedPivotState: { + activePage: DashboardState_ActivePage.PIVOT, + whereFilter: where, + pivot: { + rows: [], + columns: [ + { + id: AD_BIDS_DOMAIN_DIMENSION, + title: AD_BIDS_DOMAIN_DIMENSION, + type: PivotChipType.Dimension, + }, + { + id: AD_BIDS_BID_PRICE_MEASURE, + title: AD_BIDS_BID_PRICE_MEASURE, + type: PivotChipType.Measure, + }, + ], + sorting: [ + { + desc: true, + id: AD_BIDS_BID_PRICE_MEASURE, + }, + ], + expanded: {}, + columnPage: 1, + rowPage: 1, + enableComparison: true, + activeCell: null, + showTotalsColumn: true, + showTotalsRow: true, + tableMode: "flat", + }, + }, + }, + + { + title: "With only a single measure", + aggregationRequest: { + dimensions: [], + measures: [{ name: AD_BIDS_BID_PRICE_MEASURE }], + sort: [{ desc: true, name: AD_BIDS_BID_PRICE_MEASURE }], + }, + // Time dimension details are not opened without a time dimension + expectedNonPivotState: { + allMeasuresVisible: false, + visibleMeasures: [AD_BIDS_BID_PRICE_MEASURE], + leaderboardSortByMeasureName: AD_BIDS_BID_PRICE_MEASURE, + }, + expectedPivotState: { + activePage: DashboardState_ActivePage.PIVOT, + pivot: { + rows: [], + columns: [ + { + id: AD_BIDS_BID_PRICE_MEASURE, + title: AD_BIDS_BID_PRICE_MEASURE, + type: PivotChipType.Measure, + }, + ], + sorting: [ + { + desc: true, + id: AD_BIDS_BID_PRICE_MEASURE, + }, + ], + expanded: {}, + columnPage: 1, + rowPage: 1, + enableComparison: true, + activeCell: null, + showTotalsColumn: true, + showTotalsRow: true, + tableMode: "flat", + }, + }, + }, + ]; + + for (const { + title, + aggregationRequest, + expectedNonPivotState, + expectedPivotState, + } of TestCases) { + it(`${title} : non-pivot state`, async () => { + await runTest({ + aggregationRequest, + expectedAdditionalExploreState: expectedNonPivotState, + ignoreFilters: false, + forceOpenPivot: false, + source: NO_TIME_SOURCE, + }); + expectNoTimeRangeSummaryRequest(); + }); + + it(`${title} : pivot state`, async () => { + await runTest({ + aggregationRequest, + expectedAdditionalExploreState: expectedPivotState, + ignoreFilters: false, + forceOpenPivot: true, + source: NO_TIME_SOURCE, + }); + expectNoTimeRangeSummaryRequest(); + }); + } + }); + // TODO: add more extensive tests for other parts }); @@ -419,16 +592,52 @@ async function getExploreState( return mapQueryResp.data.exploreState; } +type TestSource = { + metricsViewName: string; + exploreName: string; + metricsView: V1MetricsViewSpec; + explore: V1ExploreSpec; + timeRangeSummary: V1TimeRangeSummary | undefined; +}; + +const AD_BIDS_SOURCE: TestSource = { + metricsViewName: AD_BIDS_METRICS_NAME, + exploreName: AD_BIDS_EXPLORE_NAME, + metricsView: AD_BIDS_METRICS_3_MEASURES_DIMENSIONS_WITH_TIME, + explore: AD_BIDS_EXPLORE_WITH_3_MEASURES_DIMENSIONS, + timeRangeSummary: AD_BIDS_TIME_RANGE_SUMMARY.timeRangeSummary, +}; + +const NO_TIME_METRICS: V1MetricsViewSpec = { + displayName: AD_BIDS_METRICS_3_MEASURES_DIMENSIONS.displayName, + table: AD_BIDS_METRICS_3_MEASURES_DIMENSIONS.table, + measures: AD_BIDS_METRICS_3_MEASURES_DIMENSIONS.measures, + dimensions: AD_BIDS_METRICS_3_MEASURES_DIMENSIONS.dimensions, +}; +const NO_TIME_EXPLORE: V1ExploreSpec = { + ...AD_BIDS_EXPLORE_WITH_3_MEASURES_DIMENSIONS, + metricsView: "AdBids_no_time_metrics", +}; +const NO_TIME_SOURCE: TestSource = { + metricsViewName: "AdBids_no_time_metrics", + exploreName: "AdBids_no_time_explore", + metricsView: NO_TIME_METRICS, + explore: NO_TIME_EXPLORE, + timeRangeSummary: undefined, +}; + async function runTest({ aggregationRequest, expectedAdditionalExploreState, ignoreFilters, forceOpenPivot, + source = AD_BIDS_SOURCE, }: { aggregationRequest: V1MetricsViewAggregationRequest; expectedAdditionalExploreState: Partial; ignoreFilters: boolean; forceOpenPivot: boolean; + source?: TestSource; }) { const mockClient = new RuntimeClient({ host: "http://localhost:9009", @@ -437,10 +646,10 @@ async function runTest({ const mapQueryStore = mapQueryToDashboard( mockClient, { - exploreName: AD_BIDS_EXPLORE_NAME, + exploreName: source.exploreName, queryName: "MetricsViewAggregation", queryArgsJson: JSON.stringify({ - metricsView: AD_BIDS_METRICS_NAME, + metricsView: source.metricsViewName, ...aggregationRequest, }), executionTime: AD_BIDS_TIME_RANGE_SUMMARY.timeRangeSummary!.max!, @@ -451,26 +660,29 @@ async function runTest({ }, ); - let mapQueryResp: MapQueryResponse | undefined; - const unsub = mapQueryStore.subscribe((r) => (mapQueryResp = r)); - await waitUntil(() => !!mapQueryResp?.data, 1000, 50); + // The store starts out undefined when its queries are already cached. + const responses: (MapQueryResponse | undefined)[] = []; + const unsub = mapQueryStore.subscribe((r) => responses.push(r)); + await waitUntil(() => !!responses.at(-1)?.data, 1000, 50); unsub(); + const mapQueryResp = responses.at(-1); if (!mapQueryResp) { throw new Error("mapQueryStore did not return a response"); } - expect(mapQueryResp.error).toBeNull(); + // No response along the way, not just the last one, should carry an error. + expect(responses.map((r) => r?.error).filter(Boolean)).toEqual([]); const rillDefaultExploreState = getRillDefaultExploreState( - AD_BIDS_METRICS_3_MEASURES_DIMENSIONS_WITH_TIME, - AD_BIDS_EXPLORE_WITH_3_MEASURES_DIMENSIONS, - AD_BIDS_TIME_RANGE_SUMMARY.timeRangeSummary, + source.metricsView, + source.explore, + source.timeRangeSummary, ); const exploreStateFromYAMLConfig = getExploreStateFromYAMLConfig( - AD_BIDS_EXPLORE_WITH_3_MEASURES_DIMENSIONS, - AD_BIDS_TIME_RANGE_SUMMARY.timeRangeSummary, - AD_BIDS_METRICS_3_MEASURES_DIMENSIONS_WITH_TIME.smallestTimeGrain, + source.explore, + source.timeRangeSummary, + source.metricsView.smallestTimeGrain, ); const expectedExploreState = { ...rillDefaultExploreState, diff --git a/web-common/src/features/explore-mappers/get-dashboard-from-aggregation-request.ts b/web-common/src/features/explore-mappers/get-dashboard-from-aggregation-request.ts index 3c2ab39cb9b9..b8b0c18a34eb 100644 --- a/web-common/src/features/explore-mappers/get-dashboard-from-aggregation-request.ts +++ b/web-common/src/features/explore-mappers/get-dashboard-from-aggregation-request.ts @@ -176,7 +176,8 @@ export async function getDashboardFromAggregationRequest({ if (req.dimensions?.length) { dashboard.selectedDimensionName = req.dimensions[0].name; dashboard.activePage = DashboardState_ActivePage.DIMENSION_TABLE; - } else { + } else if (metricsView.timeDimension) { + // Time dimension details need a time dimension to chart against. dashboard.tdd = { chartType: TDDChart.DEFAULT, expandedMeasureName: req.measures?.[0]?.name ?? "", diff --git a/web-common/src/features/explore-mappers/get-dashboard-from-comparison-request.spec.ts b/web-common/src/features/explore-mappers/get-dashboard-from-comparison-request.spec.ts new file mode 100644 index 000000000000..b6d4efa835f9 --- /dev/null +++ b/web-common/src/features/explore-mappers/get-dashboard-from-comparison-request.spec.ts @@ -0,0 +1,130 @@ +import { DashboardFetchMocks } from "@rilldata/web-common/features/dashboards/dashboard-fetch-mocks.ts"; +import { getSortType } from "@rilldata/web-common/features/dashboards/leaderboard/leaderboard-utils.ts"; +import { SortDirection } from "@rilldata/web-common/features/dashboards/proto-state/derived-types.ts"; +import { + createAndExpression, + createInExpression, +} from "@rilldata/web-common/features/dashboards/stores/filter-utils.ts"; +import { getExploreStateFromYAMLConfig } from "@rilldata/web-common/features/dashboards/stores/get-explore-state-from-yaml-config.ts"; +import { getRillDefaultExploreState } from "@rilldata/web-common/features/dashboards/stores/get-rill-default-explore-state.ts"; +import { + AD_BIDS_BID_PRICE_MEASURE, + AD_BIDS_DOMAIN_DIMENSION, + AD_BIDS_EXPLORE_INIT, + AD_BIDS_EXPLORE_NAME, + AD_BIDS_METRICS_INIT, + AD_BIDS_METRICS_NAME, + AD_BIDS_PUBLISHER_DIMENSION, +} from "@rilldata/web-common/features/dashboards/stores/test-data/data.ts"; +import { + type MapQueryResponse, + mapQueryToDashboard, +} from "@rilldata/web-common/features/explore-mappers/map-to-explore.ts"; +import { waitUntil } from "@rilldata/web-common/lib/waitUtils.ts"; +import { DashboardState_ActivePage } from "@rilldata/web-common/proto/gen/rill/ui/v1/dashboard_pb.ts"; +import { + V1MetricsViewComparisonMeasureType, + type V1MetricsViewComparisonRequest, +} from "@rilldata/web-common/runtime-client"; +import { RuntimeClient } from "@rilldata/web-common/runtime-client/v2"; +import { + afterEach, + beforeEach, + describe, + expect, + it, + type MockInstance, + vi, +} from "vitest"; + +describe("getDashboardFromComparisonRequest", () => { + const mocks = DashboardFetchMocks.useDashboardFetchMocks(); + + describe("metrics view without a time dimension", () => { + let fetchSpy: MockInstance; + + beforeEach(() => { + // AD_BIDS_METRICS_INIT has no time dimension. + mocks.mockMetricsView(AD_BIDS_METRICS_NAME, AD_BIDS_METRICS_INIT); + mocks.mockMetricsExplore( + AD_BIDS_EXPLORE_NAME, + AD_BIDS_METRICS_INIT, + AD_BIDS_EXPLORE_INIT, + ); + fetchSpy = vi.spyOn(globalThis, "fetch"); + }); + + afterEach(() => { + fetchSpy.mockRestore(); + }); + + it("With a dimension, measure and filter", async () => { + const where = createAndExpression([ + createInExpression(AD_BIDS_PUBLISHER_DIMENSION, ["Yahoo"]), + ]); + const comparisonRequest: V1MetricsViewComparisonRequest = { + metricsViewName: AD_BIDS_METRICS_NAME, + dimension: { name: AD_BIDS_DOMAIN_DIMENSION }, + measures: [{ name: AD_BIDS_BID_PRICE_MEASURE }], + sort: [ + { + name: AD_BIDS_BID_PRICE_MEASURE, + sortType: + V1MetricsViewComparisonMeasureType.METRICS_VIEW_COMPARISON_MEASURE_TYPE_BASE_VALUE, + desc: true, + }, + ], + where, + }; + + const mapQueryStore = mapQueryToDashboard( + new RuntimeClient({ + host: "http://localhost:9009", + instanceId: "default", + }), + { + exploreName: AD_BIDS_EXPLORE_NAME, + queryName: "MetricsViewComparison", + queryArgsJson: JSON.stringify(comparisonRequest), + }, + {}, + ); + + const responses: (MapQueryResponse | undefined)[] = []; + const unsub = mapQueryStore.subscribe((r) => responses.push(r)); + await waitUntil(() => !!responses.at(-1)?.data, 1000, 50); + unsub(); + + expect(responses.map((r) => r?.error).filter(Boolean)).toEqual([]); + // The runtime rejects a time range summary request for this metrics view. + expect( + fetchSpy.mock.calls.filter(([input]) => + (input instanceof Request ? input.url : input.toString()).endsWith( + "/MetricsViewTimeRange", + ), + ), + ).toEqual([]); + expect(responses.at(-1)?.data?.exploreState).toEqual({ + ...getRillDefaultExploreState( + AD_BIDS_METRICS_INIT, + AD_BIDS_EXPLORE_INIT, + undefined, + ), + ...getExploreStateFromYAMLConfig( + AD_BIDS_EXPLORE_INIT, + undefined, + AD_BIDS_METRICS_INIT.smallestTimeGrain, + ), + whereFilter: where, + visibleMeasures: [AD_BIDS_BID_PRICE_MEASURE], + leaderboardSortByMeasureName: AD_BIDS_BID_PRICE_MEASURE, + sortDirection: SortDirection.DESCENDING, + dashboardSortType: getSortType( + V1MetricsViewComparisonMeasureType.METRICS_VIEW_COMPARISON_MEASURE_TYPE_BASE_VALUE, + ), + selectedDimensionName: AD_BIDS_DOMAIN_DIMENSION, + activePage: DashboardState_ActivePage.DIMENSION_TABLE, + }); + }); + }); +}); diff --git a/web-common/src/features/explore-mappers/map-to-explore.ts b/web-common/src/features/explore-mappers/map-to-explore.ts index 89b853504712..0aaebb4343d5 100644 --- a/web-common/src/features/explore-mappers/map-to-explore.ts +++ b/web-common/src/features/explore-mappers/map-to-explore.ts @@ -15,8 +15,11 @@ import { createQueryServiceMetricsViewTimeRange, type V1MetricsViewAggregationRequest, type V1MetricsViewComparisonRequest, + type V1MetricsViewTimeRangeResponse, } from "@rilldata/web-common/runtime-client"; import type { RuntimeClient } from "@rilldata/web-common/runtime-client/v2"; +import type { ConnectError } from "@connectrpc/connect"; +import type { CreateQueryResult } from "@tanstack/svelte-query"; import { derived, readable, type Readable } from "svelte/store"; export type MapQueryRequest = { @@ -104,19 +107,40 @@ export function mapQueryToDashboard( // backwards compatibility for older alerts created on metrics explore directly if (!exploreName) exploreName = metricsViewName; + const validSpecStore = useExploreValidSpec( + client, + exploreName, + undefined, + queryClient, + ); + // Metrics views without a time dimension have no time range to fetch. + // Gated on the explore response above rather than useMetricsViewTimeRange, + // which resolves the metrics view spec through a separate GetResource call. + const timeRangeSummaryStore: CreateQueryResult< + V1MetricsViewTimeRangeResponse, + ConnectError + > = derived(validSpecStore, (validSpec, set) => + createQueryServiceMetricsViewTimeRange( + client, + { metricsViewName }, + { + query: { + enabled: !!validSpec.data?.metricsView?.timeDimension, + }, + }, + queryClient, + ).subscribe(set), + ); + return derived( - [ - useExploreValidSpec(client, exploreName, undefined, queryClient), - // TODO: handle non-timestamp dashboards - createQueryServiceMetricsViewTimeRange( - client, - { metricsViewName }, - undefined, - queryClient, - ), - ], + [validSpecStore, timeRangeSummaryStore], ([validSpecResp, timeRangeSummary], set) => { - if (validSpecResp.isLoading || timeRangeSummary.isLoading) { + const hasTimeDimension = !!validSpecResp.data?.metricsView?.timeDimension; + + if ( + validSpecResp.isLoading || + (hasTimeDimension && !timeRangeSummary.data && !timeRangeSummary.error) + ) { set({ isFetching: true, isLoading: true, @@ -125,7 +149,7 @@ export function mapQueryToDashboard( return; } - if (validSpecResp.error || timeRangeSummary.error) { + if (validSpecResp.error || (hasTimeDimension && timeRangeSummary.error)) { set({ isFetching: false, isLoading: false, @@ -151,7 +175,7 @@ export function mapQueryToDashboard( } // Type guard - if (!timeRangeSummary.data?.timeRangeSummary) { + if (hasTimeDimension && !timeRangeSummary.data?.timeRangeSummary) { set({ isFetching: false, isLoading: false, @@ -183,7 +207,7 @@ export function mapQueryToDashboard( req: queryRequestProperties, metricsView, explore, - timeRangeSummary: timeRangeSummary.data.timeRangeSummary, + timeRangeSummary: timeRangeSummary.data?.timeRangeSummary, executionTime, exploreProtoState, ignoreFilters, diff --git a/web-common/src/features/explore-mappers/types.ts b/web-common/src/features/explore-mappers/types.ts index 2d3c5871d804..c728925f5ba1 100644 --- a/web-common/src/features/explore-mappers/types.ts +++ b/web-common/src/features/explore-mappers/types.ts @@ -28,7 +28,8 @@ export type TransformerArgs = { req: R; metricsView: V1MetricsViewSpec; explore: V1ExploreSpec; - timeRangeSummary: V1TimeRangeSummary; + // Undefined when the metrics view has no time dimension. + timeRangeSummary: V1TimeRangeSummary | undefined; executionTime?: string; exploreProtoState?: string; ignoreFilters?: boolean; diff --git a/web-common/src/features/explore-mappers/utils.ts b/web-common/src/features/explore-mappers/utils.ts index 54d5c0396a4e..773583ef72dc 100644 --- a/web-common/src/features/explore-mappers/utils.ts +++ b/web-common/src/features/explore-mappers/utils.ts @@ -49,9 +49,12 @@ export async function fillTimeRange( exploreState: ExploreState, reqTimeRange: V1TimeRange | undefined, reqComparisonTimeRange: V1TimeRange | undefined, - timeRangeSummary: V1TimeRangeSummary, + timeRangeSummary: V1TimeRangeSummary | undefined, executionTime?: string, ) { + // Metrics views without a time dimension have no time range to fill. + if (!timeRangeSummary) return; + const endTime = executionTime ?? timeRangeSummary.max ?? new Date().toISOString();