diff --git a/web-admin/src/features/bookmarks/BookmarksFormDialog.svelte b/web-admin/src/features/bookmarks/BookmarksFormDialog.svelte index 36c0bf9a2510..ddfb052c1969 100644 --- a/web-admin/src/features/bookmarks/BookmarksFormDialog.svelte +++ b/web-admin/src/features/bookmarks/BookmarksFormDialog.svelte @@ -42,7 +42,6 @@ ExploreDashboardConfigProvider, } from "@rilldata/web-common/features/dashboards/providers/DashboardConfigProvider.svelte.ts"; import { onDestroy } from "svelte"; - import { syncStoreWithSource } from "@rilldata/web-common/lib/store-utils/url-params-store-sync.svelte.ts"; let { organization, @@ -77,13 +76,9 @@ dashboardConfigProvider.yamlConfigProvider, ); - // Always load from current state. This is the only route to overwrite bookmark state. + // Always load from current url state. This is the only route to overwrite bookmark state. // A future PR will improve this by adding `Replace` action, in that case this should only have bookmark's state. - syncStoreWithSource( - expressionFilterManager, - async (newUrlParams) => expressionFilterManager.setUrlParams(newUrlParams), - () => dashboardConfigProvider.metricsViewsProvider.ready, - ); + expressionFilterManager.storeSync.setUrlParams(page.url.searchParams); let timeFilterState = $state< | { diff --git a/web-admin/src/features/public-urls/CreatePublicURLForm.svelte b/web-admin/src/features/public-urls/CreatePublicURLForm.svelte index 008d4f47bff5..daa6b0577f01 100644 --- a/web-admin/src/features/public-urls/CreatePublicURLForm.svelte +++ b/web-admin/src/features/public-urls/CreatePublicURLForm.svelte @@ -38,7 +38,6 @@ ExploreDashboardConfigProvider, } from "@rilldata/web-common/features/dashboards/providers/DashboardConfigProvider.svelte.ts"; import { onDestroy } from "svelte"; - import { syncStoreWithSource } from "@rilldata/web-common/lib/store-utils/url-params-store-sync.svelte.ts"; let { dashboardResource, @@ -69,11 +68,7 @@ // Always load from current state. This is the only route to overwrite bookmark state. // A future PR will improve this by adding `Replace` action, in that case this should only have bookmark's state. - syncStoreWithSource( - expressionFilterManager, - async (newUrlParams) => expressionFilterManager.setUrlParams(newUrlParams), - () => dashboardConfigProvider.metricsViewsProvider.ready, - ); + expressionFilterManager.storeSync.setUrlParams(page.url.searchParams); const exprByMetricsView = $derived(expressionFilterManager.exprByMetricsView); const hasSomeFilter = $derived(Object.keys(exprByMetricsView).length > 0); diff --git a/web-admin/tests/embeds.spec.ts b/web-admin/tests/embeds.spec.ts index 50ef7772f1b3..fc70e78860ee 100644 --- a/web-admin/tests/embeds.spec.ts +++ b/web-admin/tests/embeds.spec.ts @@ -602,7 +602,7 @@ test.describe("Embeds", () => { ); await recorder.expectContaining( - "tr=PT6H&compare_tr=rill-PP&f.bids_metrics=advertiser_name+IN+%28%27Instacart%27%29", + "tr=PT6H&compare_tr=rill-PP&f=advertiser_name+IN+%28%27Instacart%27%29", ); }); }); diff --git a/web-common/src/features/alerts/create-alert-utils.ts b/web-common/src/features/alerts/create-alert-utils.ts index 1a61a956c219..28ed331943c3 100644 --- a/web-common/src/features/alerts/create-alert-utils.ts +++ b/web-common/src/features/alerts/create-alert-utils.ts @@ -82,7 +82,7 @@ export function getNewAlertInitialFiltersFormValues( metricsViewProvider, yamlConfigProvider, ); - filters.setUrlParams(get(page).url.searchParams); + filters.storeSync.setUrlParams(get(page).url.searchParams); const timeControls = new TimeControls(metricsViewMetadata, { selectedTimeRange: exploreState.selectedTimeRange, diff --git a/web-common/src/features/canvas/CanvasDashboardWrapper.svelte b/web-common/src/features/canvas/CanvasDashboardWrapper.svelte index e292f81d6be4..f618c704e720 100644 --- a/web-common/src/features/canvas/CanvasDashboardWrapper.svelte +++ b/web-common/src/features/canvas/CanvasDashboardWrapper.svelte @@ -9,8 +9,6 @@ import { getMissingRequiredFilters } from "@rilldata/web-common/features/dashboards/filters/utils.ts"; import MissingRequiredFiltersMessage from "@rilldata/web-common/features/dashboards/filters/MissingRequiredFiltersMessage.svelte"; import { type Snippet } from "svelte"; - import { syncStoreWithSource } from "@rilldata/web-common/lib/store-utils/url-params-store-sync.svelte.ts"; - import { goto } from "$app/navigation"; const runtimeClient = useRuntimeClient(); let instanceId = $derived(runtimeClient.instanceId); @@ -47,16 +45,6 @@ dashboardProvider, }, } = $derived(getCanvasStore(canvasName, instanceId)); - // svelte-ignore state_referenced_locally - syncStoreWithSource( - expressionFilterManager, - (newUrlParams) => { - let newSearch = newUrlParams.toString(); - if (!newSearch) newSearch = "clear=true"; - return goto("?" + newSearch); - }, - () => expressionFilterManager.metricsViewsProvider.ready, - ); $effect(() => { dashboardProvider.yamlConfigProvider.setEditable(builder); diff --git a/web-common/src/features/canvas/CanvasFilterParamsSync.svelte b/web-common/src/features/canvas/CanvasFilterParamsSync.svelte index 73677aa78cff..c90c78441c3a 100644 --- a/web-common/src/features/canvas/CanvasFilterParamsSync.svelte +++ b/web-common/src/features/canvas/CanvasFilterParamsSync.svelte @@ -2,6 +2,7 @@ import { getCanvasStore } from "@rilldata/web-common/features/canvas/state-managers/state-managers"; import { useRuntimeClient } from "@rilldata/web-common/runtime-client/v2"; import { page } from "$app/state"; + import { untrack } from "svelte"; /** * Applies the canvas state a `CanvasProvider` was given to that canvas's filter manager, once @@ -31,16 +32,16 @@ getCanvasStore(canvasName, runtimeClient.instanceId), ); - // Mirrors the `effectiveUrl` the provider applied: the override when it has one, - // the page url otherwise. - let searchParams = $derived( - urlStateOverride === undefined - ? page.url.searchParams - : new URLSearchParams(urlStateOverride), - ); - $effect(() => { - if (!canvasEntity.dashboardProvider.metricsViewsProvider.ready) return; - canvasEntity.expressionFilterManager.setUrlParams(searchParams); + // Mirrors the `effectiveUrl` the provider applied: the override when it has one, + // the page url otherwise. + const searchParams = + urlStateOverride === undefined + ? page.url.searchParams + : new URLSearchParams(urlStateOverride); + + untrack(() => { + canvasEntity.expressionFilterManager.storeSync.setUrlParams(searchParams); + }); }); diff --git a/web-common/src/features/canvas/inspector/filters/DimensionFiltersInput.svelte b/web-common/src/features/canvas/inspector/filters/DimensionFiltersInput.svelte index dd8cd0dd25a0..0c5cde5986b7 100644 --- a/web-common/src/features/canvas/inspector/filters/DimensionFiltersInput.svelte +++ b/web-common/src/features/canvas/inspector/filters/DimensionFiltersInput.svelte @@ -7,7 +7,7 @@ getParamKeyForMv, } from "@rilldata/web-common/features/dashboards/filters/ExpressionFilterManager.svelte.ts"; import VerticalExpressionFilters from "@rilldata/web-common/features/dashboards/filters/VerticalExpressionFilters.svelte"; - import { syncStoreWithSource } from "@rilldata/web-common/lib/store-utils/url-params-store-sync.svelte.ts"; + import { onMount } from "svelte"; let { id, @@ -20,30 +20,28 @@ excludedDimensions: Record; updateLocalFilterString: (newFilterString: string) => void; } = $props(); - // svelte-ignore state_referenced_locally - syncStoreWithSource( - localExpressionFilters, - async (newUrlParams) => { - localExpressionFilters.setUrlParams(newUrlParams); - updateLocalFilterString( - newUrlParams.get( - getParamKeyForMv( - localExpressionFilters.metricsViewsProvider.metricsViewNames[0], - false, - ), - ) ?? "", - ); - }, - () => localExpressionFilters.metricsViewsProvider.ready, - undefined, - true, - ); let localFiltersEnabledOverride = $state(false); let localFiltersEnabled = $derived( localExpressionFilters.hasSomeFilter || localFiltersEnabledOverride, ); + + onMount(() => { + return localExpressionFilters.storeSync.on( + "internal-change", + (newUrlParams) => { + updateLocalFilterString( + newUrlParams.get( + getParamKeyForMv( + localExpressionFilters.metricsViewsProvider.metricsViewNames[0], + false, + ), + ) ?? "", + ); + }, + ); + });
diff --git a/web-common/src/features/canvas/stores/canvas-entity.ts b/web-common/src/features/canvas/stores/canvas-entity.ts index 0b53d395d1d2..eba0a8aca50b 100644 --- a/web-common/src/features/canvas/stores/canvas-entity.ts +++ b/web-common/src/features/canvas/stores/canvas-entity.ts @@ -228,6 +228,7 @@ export class CanvasEntity { if (source && source === get(this.activeComponent)) return; this.clearActiveComponent(); }); + this.expressionFilterManager.storeSync.syncToUrl("clear=true"); this.processSpec(this.spec); } @@ -492,11 +493,7 @@ export class CanvasEntity { if (!isolated) { this.saveSnapshot(searchParams.toString()); } - // Only sync when metricsViewsProvider has loaded. Once loaded sync is handled by syncStoreWithSource - // TODO: find a good common method of sync between explore and canvas once time filters is also unified - if (this.dashboardProvider.metricsViewsProvider.ready) { - this.expressionFilterManager.setUrlParams(searchParams); - } + this.expressionFilterManager.storeSync.setUrlParams(searchParams); this.timeManager.state.onUrlChange(searchParams); this.applyTabsFromURL(searchParams); }; diff --git a/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts b/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts index 10c8cb6d13cf..67363439caa2 100644 --- a/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts +++ b/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts @@ -163,7 +163,7 @@ describe("setUrlParams", () => { it("builds a dimension chip from a per metrics view param", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, }), @@ -185,7 +185,7 @@ describe("setUrlParams", () => { it("builds a measure chip from a having param", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} having (${AD_BIDS_IMPRESSIONS_MEASURE} gt 10)`, }), @@ -230,7 +230,7 @@ describe("setUrlParams", () => { it(`builds a chip for the base measure from a ${suffix} filter`, () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} having (${AD_BIDS_IMPRESSIONS_MEASURE}${suffix} gt ${paramValue})`, }), @@ -266,7 +266,7 @@ describe("setUrlParams", () => { it(`writes a ${suffix} filter back to the param unchanged`, () => { const filterManager = createFilterManager(); const param = `${AD_BIDS_DOMAIN_DIMENSION} having (${AD_BIDS_BID_PRICE_MEASURE}${suffix} GT ${paramValue})`; - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: param }), ); @@ -283,7 +283,7 @@ describe("setUrlParams", () => { it("reads an in-list filter back as in-list mode", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN LIST ('Google','Facebook')`, }), @@ -300,7 +300,7 @@ describe("setUrlParams", () => { it("applies the singular param to every metrics view and shares one manager", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( sharedParam(`${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`), ); @@ -328,7 +328,7 @@ describe("setUrlParams", () => { }).toString(), ), }); - filterManager.setUrlParams(compressed); + filterManager.storeSync.setUrlParams(compressed); expect(names(filterManager.sortedFilterManagers.dimensions)).toEqual([ AD_BIDS_PUBLISHER_DIMENSION, @@ -348,7 +348,7 @@ describe("setUrlParams", () => { `${ExploreStateURLParams.Filters}.${AD_BIDS_MIRROR_METRICS_NAME}`, `${AD_BIDS_COUNTRY_DIMENSION} IN ('US')`, ); - filterManager.setUrlParams(searchParams); + filterManager.storeSync.setUrlParams(searchParams); // The mirror reads its own param rather than the singular one. The two are then merged into // the one filter the chips edit, and each metrics view gets the part of it that it defines: @@ -371,7 +371,7 @@ describe("setUrlParams", () => { const filterManager = createFilterManager(); // `country` belongs to the mirror only, so AdBids cannot filter on it. - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_COUNTRY_DIMENSION} IN ('US')`, }), @@ -392,7 +392,7 @@ describe("setUrlParams", () => { // `domain` is AdBids only and `country` is mirror only, so the merged filter splits back into // one condition per metrics view. - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`, [AD_BIDS_MIRROR_METRICS_NAME]: `${AD_BIDS_COUNTRY_DIMENSION} IN ('US')`, @@ -421,7 +421,7 @@ describe("setUrlParams", () => { const filterManager = createFilterManager(); // `bid_price` on `domain` is AdBids only, `publisher_count` on `country` is mirror only. - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_DOMAIN_DIMENSION} having (${AD_BIDS_BID_PRICE_MEASURE} gt 10)`, [AD_BIDS_MIRROR_METRICS_NAME]: `${AD_BIDS_COUNTRY_DIMENSION} having (${AD_BIDS_PUBLISHER_COUNT_MEASURE} lt 5)`, @@ -447,7 +447,7 @@ describe("setUrlParams", () => { it("flags an OR filter as complex", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google') OR ${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`, }), @@ -459,7 +459,7 @@ describe("setUrlParams", () => { it("flags two filters on the same dimension as complex", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google') AND ${AD_BIDS_PUBLISHER_DIMENSION} NIN ('Facebook')`, }), @@ -473,7 +473,7 @@ describe("setUrlParams", () => { // A single chip can only show one of the two, and editing it would leave the mirror on // `Facebook`, so the whole filter falls back to the read only advanced filter. - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, [AD_BIDS_MIRROR_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Facebook')`, @@ -486,7 +486,7 @@ describe("setUrlParams", () => { it("does not flag the same filter in every metrics view as complex", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, [AD_BIDS_MIRROR_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, @@ -503,7 +503,7 @@ describe("setUrlParams", () => { filterManager.addNewFilter(AD_BIDS_DOMAIN_DIMENSION); expect(filterManager.temporaryFilterName).toBe(AD_BIDS_DOMAIN_DIMENSION); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, }), @@ -516,10 +516,130 @@ describe("setUrlParams", () => { }); }); +describe("single param form", () => { + function createSingleParamFilterManager() { + const { value, destroy } = createInEffectRoot( + () => + new ExpressionFilterManager( + metricsViewsProvider, + new YAMLConfigProvider(), + true, + ), + ); + cleanups.push(destroy); + return value; + } + + it("ignores per metrics view params", () => { + const filterManager = createSingleParamFilterManager(); + + // Explore only reads the singular param, so the chips must not pick up anything else. + filterManager.storeSync.setUrlParams( + perMetricsViewParams({ + [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, + }), + ); + + expect(filterManager.sortedFilterManagers.dimensions).toEqual([]); + }); + + it("reads and writes back the singular param", () => { + const filterManager = createSingleParamFilterManager(); + const filter = `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`; + + filterManager.storeSync.setUrlParams(sharedParam(filter)); + expect(names(filterManager.sortedFilterManagers.dimensions)).toEqual([ + AD_BIDS_PUBLISHER_DIMENSION, + ]); + + const searchParams = new URLSearchParams(); + filterManager.applyFilterToParams(searchParams); + expect(searchParams.toString()).toEqual(sharedParam(filter).toString()); + }); +}); + +describe("metrics view names change", () => { + // These tests change the metrics views, so each gets its own provider. + async function createFilterManagerWithOwnProvider( + metricsViewNames: string[], + ) { + const provider = await createTestMetricsViewsProvider(metricsViewNames); + const { value, destroy } = createInEffectRoot( + () => + new ExpressionFilterManager(provider.value, new YAMLConfigProvider()), + ); + cleanups.push(() => { + destroy(); + provider.value.cleanup(); + provider.destroy(); + }); + return { provider: provider.value, filterManager: value }; + } + + it("queues params until the specs of the new metrics views load", async () => { + const { provider, filterManager } = + await createFilterManagerWithOwnProvider([AD_BIDS_METRICS_NAME]); + expect(filterManager.ready).toBe(true); + + // A metrics view that is never mocked stays loading. + provider.setMetricsViewNames([AD_BIDS_METRICS_NAME, "missing_metrics"]); + expect(provider.specsReady).toBe(false); + expect(filterManager.ready).toBe(false); + + filterManager.storeSync.setUrlParams( + sharedParam(`${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`), + ); + expect(filterManager.sortedFilterManagers.dimensions).toEqual([]); + + provider.setMetricsViewNames([AD_BIDS_METRICS_NAME]); + expect(filterManager.ready).toBe(true); + expect(names(filterManager.sortedFilterManagers.dimensions)).toEqual([ + AD_BIDS_DOMAIN_DIMENSION, + ]); + }); + + it("drops filters the new metrics views do not define", async () => { + const { provider, filterManager } = + await createFilterManagerWithOwnProvider([ + AD_BIDS_METRICS_NAME, + AD_BIDS_MIRROR_METRICS_NAME, + ]); + filterManager.storeSync.setUrlParams( + sharedParam( + `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google') AND ${AD_BIDS_COUNTRY_DIMENSION} IN ('US')`, + ), + ); + expect(names(filterManager.sortedFilterManagers.dimensions)).toEqual([ + AD_BIDS_COUNTRY_DIMENSION, + AD_BIDS_PUBLISHER_DIMENSION, + ]); + + const internalChanges: string[] = []; + cleanups.push( + filterManager.storeSync.on("internal-change", (params) => + internalChanges.push(params.toString()), + ), + ); + + // Only the mirror defines country. + provider.setMetricsViewNames([AD_BIDS_METRICS_NAME]); + await new Promise((resolve) => queueMicrotask(resolve)); + + expect(names(filterManager.sortedFilterManagers.dimensions)).toEqual([ + AD_BIDS_PUBLISHER_DIMENSION, + ]); + expect(internalChanges).toEqual([ + perMetricsViewParams({ + [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, + }).toString(), + ]); + }); +}); + describe("applyFilterToParams", () => { it("writes a param per metrics view and drops the legacy singular one", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( sharedParam(`${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`), ); @@ -540,7 +660,7 @@ describe("applyFilterToParams", () => { it("leaves nothing behind once the filter is removed", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( sharedParam(`${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`), ); filterManager.sortedFilterManagers.dimensions[0].clear(); @@ -559,7 +679,9 @@ describe("applyFilterToParams", () => { [AD_BIDS_METRICS_NAME]: `${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`, [AD_BIDS_MIRROR_METRICS_NAME]: `${AD_BIDS_COUNTRY_DIMENSION} IN ('US')`, }; - filterManager.setUrlParams(perMetricsViewParams(filterByMetricsView)); + filterManager.storeSync.setUrlParams( + perMetricsViewParams(filterByMetricsView), + ); const searchParams = new URLSearchParams(); filterManager.applyFilterToParams(searchParams); @@ -571,7 +693,7 @@ describe("applyFilterToParams", () => { it("writes back a measure filter each metrics view holds on its own", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_DOMAIN_DIMENSION} having (${AD_BIDS_BID_PRICE_MEASURE} gt 10)`, [AD_BIDS_MIRROR_METRICS_NAME]: `${AD_BIDS_COUNTRY_DIMENSION} having (${AD_BIDS_PUBLISHER_COUNT_MEASURE} lt 5)`, @@ -602,7 +724,7 @@ describe("dimension params with a binary operator", () => { it("reads EQ as a single value selection and writes it back as IN", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_DOMAIN_DIMENSION} EQ 'google.com'`, }), @@ -629,7 +751,7 @@ describe("dimension params with a binary operator", () => { it("reads NEQ as an excluded single value selection and writes it back as NIN", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_DOMAIN_DIMENSION} NEQ 'google.com'`, }), @@ -674,7 +796,7 @@ describe("dimension params with a binary operator", () => { `${AD_BIDS_DOMAIN_DIMENSION} EQ 'google.com' AND ${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, ); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: param }), ); @@ -694,7 +816,7 @@ describe("dimension params with a binary operator", () => { it("keeps an operator the chips cannot show and flags the filter as complex", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_DOMAIN_DIMENSION} GT 'g' AND ${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, }), @@ -722,7 +844,7 @@ describe("dimension params with a binary operator", () => { it("keeps the operator through the dropdown clone and drops it once the chip is edited", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_DOMAIN_DIMENSION} LTE 'g'`, }), @@ -754,7 +876,7 @@ describe("setExprForMetricsView / setParamForMetricsView", () => { it("sets the filter for one metrics view without touching the others", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( sharedParam(`${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`), ); filterManager.setParamForMetricsView( @@ -838,7 +960,7 @@ describe("setExprForMetricsView / setParamForMetricsView", () => { it("clears the filter for a metrics view when given no expression", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`, [AD_BIDS_MIRROR_METRICS_NAME]: `${AD_BIDS_COUNTRY_DIMENSION} IN ('US')`, @@ -860,7 +982,7 @@ describe("chip order", () => { yamlConfigProvider.pinnedFilters = { [AD_BIDS_PUBLISHER_DIMENSION]: true }; const filterManager = createFilterManager(yamlConfigProvider); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google') AND ${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`, [AD_BIDS_MIRROR_METRICS_NAME]: `${AD_BIDS_COUNTRY_DIMENSION} IN ('US')`, @@ -904,7 +1026,7 @@ describe("chip order", () => { it("sorts the param measures by name", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} having (${AD_BIDS_IMPRESSIONS_MEASURE} gt 10) AND ${AD_BIDS_DOMAIN_DIMENSION} having (${AD_BIDS_BID_PRICE_MEASURE} lt 2)`, }), @@ -919,7 +1041,7 @@ describe("chip order", () => { it("keys filterManagersMap by chip name", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google') AND ${AD_BIDS_DOMAIN_DIMENSION} having (${AD_BIDS_BID_PRICE_MEASURE} lt 2)`, }), @@ -977,7 +1099,7 @@ describe("addNewFilter", () => { it("does not duplicate a chip the param already covers", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, }), @@ -1027,7 +1149,7 @@ describe("dimensionFilterAction", () => { it("reuses the manager the param created", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, }), @@ -1060,7 +1182,7 @@ describe("dimensionFilterAction", () => { it("does nothing for a measure or an unknown name", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} having (${AD_BIDS_IMPRESSIONS_MEASURE} gt 10)`, }), @@ -1097,12 +1219,7 @@ describe("dimensionFilterAction", () => { AD_BIDS_PUBLISHER_DIMENSION, (manager) => manager.toggleValue("Google", false), ); - expect( - filterManager.sortedFilterManagers.dimensions[0], - ).not.toBeUndefined(); - expect( - filterManager.sortedFilterManagers.dimensions[0].selectedValues, - ).toEqual([]); + expect(filterManager.sortedFilterManagers.dimensions[0]).toBeUndefined(); filterManager.dimensionFilterAction( AD_BIDS_PUBLISHER_DIMENSION, @@ -1122,7 +1239,7 @@ describe("dimensionFilterAction", () => { describe("with a Contains filter applied", () => { function createWithContainsFilter() { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} LIKE '%oo%'`, }), @@ -1161,7 +1278,7 @@ describe("dimensionFilterAction", () => { it("toggleValue keeps exclude when converting", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} NLIKE '%oo%'`, }), @@ -1219,7 +1336,7 @@ describe("clear", () => { it("removes every chip and the temporary filter", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, [AD_BIDS_MIRROR_METRICS_NAME]: `${AD_BIDS_COUNTRY_DIMENSION} IN ('US')`, @@ -1240,7 +1357,7 @@ describe("clear", () => { yamlConfigProvider.requiredFilters = { [AD_BIDS_DOMAIN_DIMENSION]: true }; const filterManager = createFilterManager(yamlConfigProvider); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`, }), @@ -1259,7 +1376,7 @@ describe("clear", () => { describe("getOtherDimensionsFilter", () => { function setupThreeDimensions() { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google') AND ${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`, [AD_BIDS_MIRROR_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google') AND ${AD_BIDS_COUNTRY_DIMENSION} IN ('US')`, @@ -1302,7 +1419,7 @@ describe("getOtherDimensionsFilter", () => { it("returns undefined when no other dimension is filtered", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, }), @@ -1354,10 +1471,12 @@ describe("createLocalFilterStore", () => { expect(local.yamlConfigProvider).toBe(yamlConfigProvider); // The mirror-only dimension is out of scope for the local store. - local.setUrlParams(sharedParam(`${AD_BIDS_COUNTRY_DIMENSION} IN ('US')`)); + local.storeSync.setUrlParams( + sharedParam(`${AD_BIDS_COUNTRY_DIMENSION} IN ('US')`), + ); expect(local.sortedFilterManagers.dimensions).toEqual([]); - local.setUrlParams( + local.storeSync.setUrlParams( sharedParam(`${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`), ); expect(names(local.sortedFilterManagers.dimensions)).toEqual([ @@ -1374,7 +1493,7 @@ describe("exprByMetricsView", () => { it("builds an expression per metrics view", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google') AND ${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`, [AD_BIDS_MIRROR_METRICS_NAME]: `${AD_BIDS_COUNTRY_DIMENSION} IN ('US')`, @@ -1398,7 +1517,7 @@ describe("exprByMetricsView", () => { it("omits metrics views with no filter", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`, }), @@ -1438,7 +1557,7 @@ describe("exprByMetricsView", () => { it("mirrors exprByMetricsView into the svelte 4 store", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`, }), @@ -1465,7 +1584,7 @@ describe("filter-changed", () => { it("reports a chip edit with no source", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, }), @@ -1523,7 +1642,7 @@ describe("filter-changed", () => { it("reports clear with no source", () => { const filterManager = createFilterManager(); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google')`, }), @@ -1539,7 +1658,7 @@ describe("filter-changed", () => { const filterManager = createFilterManager(); const sources = recordSources(filterManager); - filterManager.setUrlParams( + filterManager.storeSync.setUrlParams( perMetricsViewParams({ [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google') AND ${AD_BIDS_PUBLISHER_DIMENSION} having (${AD_BIDS_IMPRESSIONS_MEASURE} gt 10)`, }), diff --git a/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts b/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts index f0ba78866877..4c68d264c090 100644 --- a/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts +++ b/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts @@ -22,8 +22,10 @@ import type { } from "@rilldata/web-common/features/dashboards/filters/filter-events.ts"; import { mergeFilterParams } from "@rilldata/web-common/features/dashboards/filters/expr-utils.ts"; import { getSortFilterManagers } from "@rilldata/web-common/features/dashboards/filters/get-sort-filter-managers.ts"; -import { expandCompressedParams } from "@rilldata/web-common/features/dashboards/url-state/compression.ts"; -import type { UrlParamsStore } from "@rilldata/web-common/lib/store-utils/url-params-store-sync.svelte.ts"; +import { + UrlParamsChangeTracker, + type UrlParamsStore, +} from "@rilldata/web-common/lib/store-utils/url-params-store-sync.svelte.ts"; /** * Filter managers for the chips in a filter bar. @@ -52,8 +54,6 @@ export class ExpressionFilterManager implements UrlParamsStore { }; public readonly filterManagersMap: Record; - private curParams = $state(undefined); - // Shared with every manager below this one, so a chip edit reports itself // without the managers in between having to forward it. See `filter-events.ts`. private events = new EventEmitter(); @@ -61,14 +61,47 @@ export class ExpressionFilterManager implements UrlParamsStore { this.events, ) as typeof this.events.on; - // Temporary lock in explore. Once we move whereFilter out of explore, we can remove this. - public updating = false; + public ready = $state(false); + + public paramKeys = new Set([ExploreStateURLParams.Filters]); + public readonly storeSync: UrlParamsChangeTracker; + + // Unsubscribes from `metricsViewsProvider`, which can outlive this manager. See `cleanup`. + private readonly unsubscribers: (() => void)[]; public constructor( public readonly metricsViewsProvider: MetricsViewsProvider, public readonly yamlConfigProvider: YAMLConfigProvider, private readonly singleParamFormMv = false, ) { + this.storeSync = new UrlParamsChangeTracker(this); + + const syncParamKeys = (names: string[]) => { + this.paramKeys = new Set([ + ExploreStateURLParams.Filters, + ...names.map((mvName) => getParamKeyForMv(mvName, singleParamFormMv)), + ]); + }; + const unsubUpdate = metricsViewsProvider.on( + "update-metrics-views", + (names) => { + syncParamKeys(names); + // Specs for the new metrics views are loading, so params are queued until `specs-loaded`. + this.ready = metricsViewsProvider.specsReady; + }, + ); + // Call sync immediately for already loaded metricsViewsProvider + syncParamKeys(metricsViewsProvider.metricsViewNames); + + this.ready = metricsViewsProvider.specsReady; + // Emit ready immediately for already loaded metricsViewsProvider + if (metricsViewsProvider.specsReady) this.events.emit("ready"); + const unsubSpecsLoaded = metricsViewsProvider.on("specs-loaded", () => { + this.ready = true; + this.events.emit("ready"); + }); + this.unsubscribers = [unsubUpdate, unsubSpecsLoaded]; + this.topLevelJoiner = $state( JoinerFilterManager.parse( this.metricsViewsProvider, @@ -78,9 +111,9 @@ export class ExpressionFilterManager implements UrlParamsStore { this.events, ) as JoinerFilterManager, ); - this.on("filter-removed", ({ name, wasEmpty }) => { - if (!wasEmpty) return; // This will change expr and other pipelines will update managers. - this.topLevelJoiner.removeManagerByName(name); + this.on("filter-changed", () => { + this.topLevelJoiner?.removeEmptyManagers(); + this.storeSync.stateChanged(); }); this.sortedFilterManagers = $derived.by(() => @@ -118,39 +151,35 @@ export class ExpressionFilterManager implements UrlParamsStore { const newUrlSearch = new URLSearchParams(); this.applyFilterToParams(newUrlSearch); - cloned.setUrlParams(newUrlSearch); + cloned.storeSync.setUrlParams(newUrlSearch); return cloned; } - public setUrlParams(searchParams: URLSearchParams) { - let expandedUrlParams: URLSearchParams; - try { - expandedUrlParams = expandCompressedParams(searchParams); - } catch { - // If we fail to decompress, do not throw here. - return; - } + /** + * Removes the listeners on `metricsViewsProvider`. + * Call this when the manager is discarded before the provider, e.g. a temporary `clone`. + */ + public cleanup() { + this.unsubscribers.forEach((unsub) => unsub()); + } - // Use and save just the params set by this class. - const relevantUrlParams = new URLSearchParams(); - expandedUrlParams.forEach((value, key) => { - if ( - key === ExploreStateURLParams.Filters || - key.startsWith(ExploreStateURLParams.Filters + ".") - ) { - relevantUrlParams.append(key, value); - } - }); + public normalizeParams(urlParams: URLSearchParams): URLSearchParams { + const singularParam = urlParams.get(ExploreStateURLParams.Filters); + if (!singularParam || this.singleParamFormMv) return urlParams; - // Do not update managers if params didnt change. - if ( - this.curParams && - this.curParams.toString() === relevantUrlParams.toString() - ) - return; + const newUrlParams = new URLSearchParams(); + this.metricsViewsProvider.metricsViewNames.forEach((mvName) => + newUrlParams.set( + getParamKeyForMv(mvName, this.singleParamFormMv), + singularParam, + ), + ); + return newUrlParams; + } - const { expr, inList, advanced } = mergeFilterParams(relevantUrlParams); + public setUrlParams(searchParams: URLSearchParams) { + const { expr, inList, advanced } = mergeFilterParams(searchParams); this.temporaryFilterName = undefined; this.topLevelJoiner = JoinerFilterManager.parse( @@ -161,19 +190,14 @@ export class ExpressionFilterManager implements UrlParamsStore { this.events, ) as JoinerFilterManager; this.isComplexFilter = advanced; - - this.curParams = normalizeUrlParams( - relevantUrlParams, - this.metricsViewsProvider.metricsViewNames, - this.singleParamFormMv, - ); } public setParamForMetricsView(mvName: string, param: string) { const paramKey = getParamKeyForMv(mvName, this.singleParamFormMv); - const newParams = new URLSearchParams(this.curParams); + const newParams = new URLSearchParams(this.storeSync.searchParams); newParams.set(paramKey, param); - this.setUrlParams(newParams); + // Thread through the sync code to ensure only changes update the internal state. + this.storeSync.setUrlParams(newParams); } public setExprForMetricsView( @@ -201,7 +225,7 @@ export class ExpressionFilterManager implements UrlParamsStore { // A singular `f=...` is a legacy canvas filter. `setUrlParams` folds it into every metrics view, // so the per metrics view params written above already carry it and it can be dropped. - if (!this.singleParamFormMv && this.metricsViewsProvider.ready) { + if (!this.singleParamFormMv && this.metricsViewsProvider.specsReady) { searchParams.delete(ExploreStateURLParams.Filters); } } @@ -337,24 +361,6 @@ export class ExpressionFilterManager implements UrlParamsStore { } } -function normalizeUrlParams( - urlParams: URLSearchParams, - metricsViews: string[], - singleParamFormMv: boolean, -) { - const singularParam = urlParams.get(ExploreStateURLParams.Filters); - if (!singularParam || singleParamFormMv) return urlParams; - - const newUrlParams = new URLSearchParams(); - metricsViews.forEach((mvName) => - newUrlParams.set( - getParamKeyForMv(mvName, singleParamFormMv), - singularParam, - ), - ); - return newUrlParams; -} - export function getParamKeyForMv(mvName: string, singleParamFormMv: boolean) { return singleParamFormMv ? ExploreStateURLParams.Filters diff --git a/web-common/src/features/dashboards/filters/Filters.svelte b/web-common/src/features/dashboards/filters/Filters.svelte index 9393813a10e0..e1f145b09bb1 100644 --- a/web-common/src/features/dashboards/filters/Filters.svelte +++ b/web-common/src/features/dashboards/filters/Filters.svelte @@ -42,7 +42,6 @@ import { createAndExpression } from "@rilldata/web-common/features/dashboards/stores/filter-utils.ts"; import { untrack } from "svelte"; import type { ExploreState } from "@rilldata/web-common/features/dashboards/stores/explore-state"; - import { syncStoreWithSource } from "@rilldata/web-common/lib/store-utils/url-params-store-sync.svelte.ts"; const { rillTime } = featureFlags; @@ -69,15 +68,6 @@ expressionFilterManager, } = StateManagers; - syncStoreWithSource( - expressionFilterManager, - syncExpressionFilters, - () => expressionFilterManager.metricsViewsProvider.ready, - undefined, - // URL sync is managed by DashboardStateSync - true, - ); - const timeControlsStore = useTimeControlStore(StateManagers); const dashboardStateSync = DashboardStateSync.getFromContext(); @@ -394,20 +384,12 @@ dimensionsWithInlistFilter: tempFilterManger.inList, }; + tempFilterManger.cleanup(); + const url = dashboardStateSync.getUrlForExploreState(exploreState); return isUrlTooLong(url); }); } - - function syncExpressionFilters() { - if (!expressionFilterManager.updating) { - metricsExplorerStore.syncExpressionFilter( - $exploreName, - expressionFilterManager, - ); - } - return Promise.resolve(); - }
diff --git a/web-common/src/features/dashboards/filters/JoinerFilterManager.svelte.ts b/web-common/src/features/dashboards/filters/JoinerFilterManager.svelte.ts index 9b83fa42c1f0..d378cce10027 100644 --- a/web-common/src/features/dashboards/filters/JoinerFilterManager.svelte.ts +++ b/web-common/src/features/dashboards/filters/JoinerFilterManager.svelte.ts @@ -268,14 +268,20 @@ export class JoinerFilterManager { }; } - public removeManagerByName(name: string) { + public removeEmptyManagers() { this.managers = { ...this.managers, dimensionManagers: this.managers.dimensionManagers.filter( - (dfm) => dfm.name !== name, + (dfm) => + !!dfm.expr || + this.yamlConfigProvider?.requiredFilters[dfm.name] || + this.yamlConfigProvider?.pinnedFilters[dfm.name], ), measureManagers: this.managers.measureManagers.filter( - (mfm) => mfm.name !== name, + (mfm) => + !!mfm.expr || + this.yamlConfigProvider?.requiredFilters[mfm.name] || + this.yamlConfigProvider?.pinnedFilters[mfm.name], ), }; } diff --git a/web-common/src/features/dashboards/filters/dimension-filters/DimensionFilterManager.svelte.ts b/web-common/src/features/dashboards/filters/dimension-filters/DimensionFilterManager.svelte.ts index 7b420269bc79..cec7f5a5c16f 100644 --- a/web-common/src/features/dashboards/filters/dimension-filters/DimensionFilterManager.svelte.ts +++ b/web-common/src/features/dashboards/filters/dimension-filters/DimensionFilterManager.svelte.ts @@ -228,12 +228,7 @@ export class DimensionFilterManager { this.rawExpr = undefined; this.selectedValues = []; this.inputText = ""; - const wasEmpty = this.expr === undefined; this.commit(); - this.events?.emit("filter-removed", { - name: this.name, - wasEmpty, - }); } /** diff --git a/web-common/src/features/dashboards/filters/filter-events.ts b/web-common/src/features/dashboards/filters/filter-events.ts index 0501aefb7e35..0aeb07f9b93e 100644 --- a/web-common/src/features/dashboards/filters/filter-events.ts +++ b/web-common/src/features/dashboards/filters/filter-events.ts @@ -10,9 +10,7 @@ export type FilterChangeSource = string | undefined; export type FilterEvents = { // A filter was mutated. Emitted synchronously by the manager that was mutated. "filter-changed": { source: FilterChangeSource }; - // Emitted when a filter is manually removed from a manager. - // This is useful when removal doesn't change expr and the manager has to be manually removed. - "filter-removed": { name: string; wasEmpty: boolean }; + ready: void; }; /** diff --git a/web-common/src/features/dashboards/filters/measure-filters/MeasureFilterManager.svelte.ts b/web-common/src/features/dashboards/filters/measure-filters/MeasureFilterManager.svelte.ts index 0b957834f26f..4644fe91c15f 100644 --- a/web-common/src/features/dashboards/filters/measure-filters/MeasureFilterManager.svelte.ts +++ b/web-common/src/features/dashboards/filters/measure-filters/MeasureFilterManager.svelte.ts @@ -99,12 +99,7 @@ export class MeasureFilterManager { this.value1 = ""; this.value2 = ""; this.dimension = ""; - const wasEmpty = this.expr === undefined; this.commit(); - this.events?.emit("filter-removed", { - name: this.name, - wasEmpty, - }); } /** diff --git a/web-common/src/features/dashboards/filters/test/StandaloneExpressionFiltersTest.svelte b/web-common/src/features/dashboards/filters/test/StandaloneExpressionFiltersTest.svelte index ae88990dc4fc..30d7ef31feff 100644 --- a/web-common/src/features/dashboards/filters/test/StandaloneExpressionFiltersTest.svelte +++ b/web-common/src/features/dashboards/filters/test/StandaloneExpressionFiltersTest.svelte @@ -3,7 +3,6 @@ import ExpressionFilters from "@rilldata/web-common/features/dashboards/filters/ExpressionFilters.svelte"; import { YAMLConfigProvider } from "@rilldata/web-common/features/dashboards/providers/YAMLConfigProvider.svelte.ts"; import { MetricsViewsProvider } from "@rilldata/web-common/features/metrics-views/providers/MetricsViewsProvider.svelte.ts"; - import { syncStoreWithSource } from "@rilldata/web-common/lib/store-utils/url-params-store-sync.svelte.ts"; import { useRuntimeClient } from "@rilldata/web-common/runtime-client/v2"; /** @@ -33,15 +32,11 @@ new YAMLConfigProvider(), ); // svelte-ignore state_referenced_locally + metricsViewsProvider.setMetricsViewNames(metricsViewNames); + // svelte-ignore state_referenced_locally onManagerCreated?.(expressionFilterManager); - syncStoreWithSource( - expressionFilterManager, - async (newUrlParams) => expressionFilterManager.setUrlParams(newUrlParams), - () => metricsViewsProvider.ready, - undefined, - true, - ); + expressionFilterManager.storeSync.setUrlParams(new URLSearchParams()); (DASHBOARD_STATE_SYNC_KEY); } @@ -78,6 +82,8 @@ export class DashboardStateSync { dataLoader.fullTimeRangeQuery, ); + this.expressionFilterParamsTracker = expressionFilterManager.storeSync; + this.unsubInit = derived( [dataLoader.initExploreState], (states) => states, @@ -171,7 +177,6 @@ export class DashboardStateSync { // Ensure dashboard data is loaded before we proceed. if (!rillDefaultExploreURLParams) return; this.updating = true; - this.expressionFilterManager.updating = true; const pageState = get(page); @@ -226,8 +231,7 @@ export class DashboardStateSync { } this.lastEphemeralMeasures = initExploreState.ephemeralMeasures; - this.expressionFilterManager.setUrlParams(redirectUrl.searchParams); - this.expressionFilterManager.updating = false; + this.expressionFilterParamsTracker.setUrlParams(redirectUrl.searchParams); // If the current url same as the new url then there is no need to do anything if (redirectUrl.search === pageState.url.search) { this.initialized = true; @@ -283,7 +287,6 @@ export class DashboardStateSync { // Take the lock only once the guards have passed; // the finally ensures a throw below cannot leave it stuck. this.updating = true; - this.expressionFilterManager.updating = true; let redirectUrl: URL | undefined = undefined; // TODO: reassess this try-catch. resolveTimeRanges has error handling already. try { @@ -309,7 +312,6 @@ export class DashboardStateSync { metricsExplorerStore.mergePartialExplorerEntity( this.exploreName, partialExplore, - this.expressionFilterManager, ); // Get time controls state after explore state is updated. const timeControlsState = get(this.timeControlStore); @@ -345,9 +347,10 @@ export class DashboardStateSync { // must still be picked up by gotoNewState. this.updating = false; if (redirectUrl) { - this.expressionFilterManager.setUrlParams(redirectUrl.searchParams); + this.expressionFilterParamsTracker.setUrlParams( + redirectUrl.searchParams, + ); } - this.expressionFilterManager.updating = false; } // Try-finally without a catch. Rest of the code is not run if the above try body throws. @@ -377,7 +380,6 @@ export class DashboardStateSync { // Those methods need to replace the current URL while this does a direct navigation. if (this.updating) return; this.updating = true; - this.expressionFilterManager.updating = true; try { const { data: validSpecData } = get(this.dataLoader.validSpecQuery); @@ -413,7 +415,7 @@ export class DashboardStateSync { ); } - this.expressionFilterManager.setUrlParams(newUrl.searchParams); + this.expressionFilterParamsTracker.setUrlParams(newUrl.searchParams); // If the state didnt result in a new url then skip goto. // This avoids adding redundant urls to the history. if (newUrl.search === pageState.url.search) { @@ -424,7 +426,6 @@ export class DashboardStateSync { await goto(newUrl); } finally { this.updating = false; - this.expressionFilterManager.updating = false; } } } diff --git a/web-common/src/features/dashboards/state-managers/state-managers.ts b/web-common/src/features/dashboards/state-managers/state-managers.ts index fef016d45c09..bded7890fa04 100644 --- a/web-common/src/features/dashboards/state-managers/state-managers.ts +++ b/web-common/src/features/dashboards/state-managers/state-managers.ts @@ -171,10 +171,18 @@ export function createStateManagers({ runtimeClient, exploreName, ); + // Explore urls only carry the singular `f` param, so per metrics view params are ignored here. const expressionFilterManager = new ExpressionFilterManager( dashboardProvider.metricsViewsProvider, dashboardProvider.yamlConfigProvider, + true, ); + expressionFilterManager.storeSync.on("internal-change", () => { + metricsExplorerStore.syncExpressionFilter( + exploreName, + expressionFilterManager, + ); + }); return { runtimeClient, diff --git a/web-common/src/features/dashboards/stores/dashboard-stores.ts b/web-common/src/features/dashboards/stores/dashboard-stores.ts index 20351c490992..bd4a98ea6314 100644 --- a/web-common/src/features/dashboards/stores/dashboard-stores.ts +++ b/web-common/src/features/dashboards/stores/dashboard-stores.ts @@ -241,7 +241,7 @@ const metricsViewReducers = { mergePartialExplorerEntity( name: string, partialExploreState: Partial, - expressionFilterManager: ExpressionFilterManager, + expressionFilterManager?: ExpressionFilterManager, ) { partialExploreState = structuredClone(partialExploreState); @@ -250,14 +250,16 @@ const metricsViewReducers = { exploreState[key] = partialExploreState[key]; } - const mvName = - expressionFilterManager.metricsViewsProvider.metricsViewNames[0]; - if (mvName) { - exploreState.whereFilter = - expressionFilterManager.topLevelJoiner.expr[mvName] ?? - createAndExpression([]); - exploreState.dimensionsWithInlistFilter = - expressionFilterManager.inList; + if (expressionFilterManager) { + const mvName = + expressionFilterManager.metricsViewsProvider.metricsViewNames[0]; + if (mvName) { + exploreState.whereFilter = + expressionFilterManager.topLevelJoiner.expr[mvName] ?? + createAndExpression([]); + exploreState.dimensionsWithInlistFilter = + expressionFilterManager.inList; + } } // this hack is needed since what is shown for comparison is not a single source diff --git a/web-common/src/features/dashboards/url-state/test/url-state-test-utils.ts b/web-common/src/features/dashboards/url-state/test/url-state-test-utils.ts index 895fdd5a3113..e680ca484741 100644 --- a/web-common/src/features/dashboards/url-state/test/url-state-test-utils.ts +++ b/web-common/src/features/dashboards/url-state/test/url-state-test-utils.ts @@ -50,6 +50,7 @@ export function useTestFilterManager(specs: MetricsViewSpecs) { new ExpressionFilterManager( metricsViewsProvider, new YAMLConfigProvider(), + true, ), ); filterManager = created.value; @@ -69,7 +70,7 @@ export function applyURLToExploreState( defaultExplorePreset: V1ExplorePreset, filterManager: ExpressionFilterManager, ) { - filterManager.setUrlParams(url.searchParams); + filterManager.storeSync.setUrlParams(url.searchParams); const { partialExploreState: partialExploreStateDefaultUrl, errors } = convertURLSearchParamsToExploreState( diff --git a/web-common/src/features/metrics-views/providers/MetricsViewsProvider.svelte.ts b/web-common/src/features/metrics-views/providers/MetricsViewsProvider.svelte.ts index 08301a3f119c..3eb0049165b0 100644 --- a/web-common/src/features/metrics-views/providers/MetricsViewsProvider.svelte.ts +++ b/web-common/src/features/metrics-views/providers/MetricsViewsProvider.svelte.ts @@ -13,11 +13,18 @@ import { Duration } from "luxon"; import { queryClient } from "@rilldata/web-common/lib/svelte-query/globalQueryClient.ts"; import { ResourceKind } from "@rilldata/web-common/features/entity-management/resource-selectors.ts"; import { arrayUnorderedEquals } from "@rilldata/web-common/lib/arrayUtils.ts"; +import { EventEmitter } from "@rilldata/web-common/lib/event-emitter.ts"; export type MetricsViewName = string; export type DimensionName = string; export type MeasureName = string; +type MetricsViewsProviderEvents = { + "update-metrics-views": string[]; + "specs-loaded": void; + "time-specs-loaded": void; +}; + /** * Reactive view over a set of metrics views. * @@ -61,19 +68,24 @@ export class MetricsViewsProvider { /** Smallest restriction across the metrics views, since it has to hold for all of them. */ public maxQueryTimeRange: Duration | undefined; /** True once every metrics view has a spec and every time series metrics view has a summary. */ + public specsReady = $state(false); public ready: boolean; public metricsViewNames = $state([]); public cleanup: () => void; + private events = new EventEmitter(); + public readonly on = this.events.on.bind(this.events); + private resources: V1Resource[] = []; private readonly timeRangeUnsubs = new Map void>(); + private pendingSpecs = new Set(); public constructor( public readonly runtimeClient: RuntimeClient, initMetricsViewNames: string[], ) { - this.metricsViewNames = initMetricsViewNames.filter(Boolean); + this.setMetricsViewNames(initMetricsViewNames); const allResourcesQuery = createRuntimeServiceListResources( runtimeClient, @@ -167,6 +179,11 @@ export class MetricsViewsProvider { }); this.metricsViewNames = metricsViewNames; + // The specs for the new set have to load before dependents can use them. + // `processResources` below marks them ready again if they are already available. + this.specsReady = false; + this.pendingSpecs = new Set(metricsViewNames); + this.events.emit("update-metrics-views", this.metricsViewNames); this.processResources(); } @@ -234,6 +251,10 @@ export class MetricsViewsProvider { this.simpleMeasures = simpleMeasures; this.dimensionSpecs = dimensionSpecs; this.dimensions = dimensions; + + Object.keys(specs).forEach((metricsViewName) => + this.specLoaded(metricsViewName), + ); } /** @@ -265,4 +286,13 @@ export class MetricsViewsProvider { }), ); } + + private specLoaded(name: string) { + if (!this.pendingSpecs.has(name)) return; + this.pendingSpecs.delete(name); + + if (this.pendingSpecs.size > 0) return; + this.events.emit("specs-loaded"); + this.specsReady = true; + } } diff --git a/web-common/src/features/scheduled-reports/FiltersForm.svelte b/web-common/src/features/scheduled-reports/FiltersForm.svelte index b31049a8c12d..d1e7cf41e3a8 100644 --- a/web-common/src/features/scheduled-reports/FiltersForm.svelte +++ b/web-common/src/features/scheduled-reports/FiltersForm.svelte @@ -22,7 +22,6 @@ import type { ExpressionFilterManager } from "../dashboards/filters/ExpressionFilterManager.svelte.ts"; import { useExploreValidSpec } from "@rilldata/web-common/features/explores/selectors.ts"; import ExpressionFilters from "../dashboards/filters/ExpressionFilters.svelte"; - import { syncStoreWithSource } from "@rilldata/web-common/lib/store-utils/url-params-store-sync.svelte.ts"; const runtimeClient = useRuntimeClient(); @@ -43,17 +42,6 @@ side?: "top" | "right" | "bottom" | "left"; } = $props(); - // The filters belong to the report or alert in the form, not to the page the form is opened on. - // Syncing with the page URL would replace them with the filters in that URL, which are usually none. - // svelte-ignore state_referenced_locally - syncStoreWithSource( - filters, - async (newUrlParams) => filters.setUrlParams(newUrlParams), - () => filters.metricsViewsProvider.ready, - undefined, - true, - ); - let { selectedTimezone, allTimeRange: allTimeRangeStore, diff --git a/web-common/src/lib/store-utils/url-params-store-sync.svelte.ts b/web-common/src/lib/store-utils/url-params-store-sync.svelte.ts index 5674ca92c3c0..ea9118a8d0b6 100644 --- a/web-common/src/lib/store-utils/url-params-store-sync.svelte.ts +++ b/web-common/src/lib/store-utils/url-params-store-sync.svelte.ts @@ -1,102 +1,159 @@ -import { cleanUrlParams } from "@rilldata/web-common/features/dashboards/url-state/clean-url-params.ts"; +import { EventEmitter } from "@rilldata/web-common/lib/event-emitter.ts"; +import { expandCompressedParams } from "@rilldata/web-common/features/dashboards/url-state/compression.ts"; +import { copySubsetParams } from "@rilldata/web-common/lib/url-utils.ts"; +import { goto } from "$app/navigation"; import { page } from "$app/state"; -import { untrack } from "svelte"; + +type UrlParamsStoreEvents = { + ready: void; +}; export interface UrlParamsStore { setUrlParams(urlParams: URLSearchParams): void; applyFilterToParams(urlParams: URLSearchParams): void; + ready: boolean; + + on: EventEmitter["on"]; + + /** + * Url param keys set by this class. + */ + paramKeys: Set; + normalizeParams(urlParams: URLSearchParams): URLSearchParams; } -export function syncStoreWithSource( - store: UrlParamsStore, - sync: (newUrlParams: URLSearchParams) => Promise, - readyGetter: () => boolean, - defaultUrlParamsGetter?: () => URLSearchParams | undefined, - skipUrlSync = false, -) { - let lock = false; - - if (!skipUrlSync) { - $effect(() => { - // Read all dependencies first so the subscription survives the guard. - const currentUrl = page.url; - const defaultUrlParams = untrack(() => - defaultUrlParamsGetter ? defaultUrlParamsGetter() : undefined, - ); - const ready = readyGetter(); - - if (!ready || lock) return; - lock = true; - - const newUrlParams = new URLSearchParams(currentUrl.searchParams); - if (defaultUrlParams) { - defaultUrlParams.forEach((value, key) => { - if (newUrlParams.has(key)) return; - newUrlParams.set(key, value); - }); - } - - // No need to safeguard against unchanged url. - // It should already happen in setUrlParams since it will have other callers. - untrack(() => store.setUrlParams(newUrlParams)); - - lock = false; - }); - } +type UrlParamsChangeTrackerEvents = { + /** + * Fired when the underlying class's state changes internally. + * Happens when UI controls directly update class's state. + * Does not fire when `setUrlParams` is called to avoid loop. + */ + "internal-change": URLSearchParams; + /** + * Fired when + */ + change: URLSearchParams; +}; - let prevStateParams = new URLSearchParams(); - $effect(() => { - // Read all dependencies first so the subscription survives the guard. - const curStateParams = new URLSearchParams(); - store.applyFilterToParams(curStateParams); +/** + * A class that tracks URL search params that splits into multiple stores but backed by a single class. + * + * External sources like navigation should call `setUrlParams` and listen to `change` to sync. + * The internal store class that expands the search params into multiple stores should call `stateChanged` and listens to `set` to sync. + */ +export class UrlParamsChangeTracker { + /** + * Source of truth for the underlying class's state. + */ + public searchParams = $state(); - const defaultUrlParams = untrack(() => - defaultUrlParamsGetter ? defaultUrlParamsGetter() : undefined, - ); - const ready = readyGetter(); + private pendingParams: URLSearchParams | undefined = undefined; + + private events = new EventEmitter(); + public readonly on = this.events.on.bind( + this.events, + ) as typeof this.events.on; + + public constructor(private readonly store: UrlParamsStore) { + this.store.on("ready", () => this.replayPendingParams()); + } - if ( - !ready || - lock || - curStateParams.toString() === prevStateParams.toString() - ) + /** + * Called by external sources like navigation to sync the search params. + * Calls `setUrlParams` on the store class. + */ + public setUrlParams(urlParams: URLSearchParams) { + if (!this.store.ready) { + this.pendingParams = urlParams; return; - lock = true; + } + + let expandedUrlParams: URLSearchParams; + try { + expandedUrlParams = expandCompressedParams(urlParams); + } catch { + // If we fail to decompress, do not throw here. + return; + } - const currentUrlParams = untrack(() => - skipUrlSync - ? new URLSearchParams(prevStateParams) - : page.url.searchParams, + const relevantParams = copySubsetParams( + expandedUrlParams, + this.store.paramKeys, ); - prevStateParams = curStateParams; + if (this.searchParams?.toString() === relevantParams.toString()) return; - let newUrlParams = new URLSearchParams(currentUrlParams); - if (defaultUrlParams) { - newUrlParams = cleanUrlParams(newUrlParams, defaultUrlParams); - } - untrack(() => { - store.applyFilterToParams(newUrlParams); + this.searchParams = this.store.normalizeParams(relevantParams); + this.store.setUrlParams(relevantParams); + this.events.emit("change", this.searchParams); + } + + /** + * Called by internal store class when its state is directly changed like a user action. + * Calls `applyFilterToParams` to get the actual url params in a new microtask so that changes are propagated. + * Fires `change` event so that external sources like navigation can sync. + */ + public stateChanged() { + queueMicrotask(() => this.maybeNotifyStateChange()); + } + + /** + * Syncs the store's state to the URL by listening to 'change' event. + * Returns the unsub method from the event listener. It is the callers' responsibility to call it. + * @param emptySearchOverride The search override to use if the store's state is empty. + */ + public syncToUrl(emptySearchOverride = "") { + return this.on("internal-change", (newUrlParams) => { + const urlParamsToApply = new URLSearchParams(page.url.searchParams); + this.store.paramKeys.forEach((key) => { + if (newUrlParams.has(key)) { + urlParamsToApply.set(key, newUrlParams.get(key) ?? ""); + } else { + urlParamsToApply.delete(key); + } + }); + + let searchToApply = urlParamsToApply.toString(); + if (!searchToApply) searchToApply = emptySearchOverride; + void goto("?" + searchToApply); }); + } - if (newUrlParams.toString() === currentUrlParams.toString()) { - lock = false; + private replayPendingParams() { + if (!this.pendingParams) { + this.reapplyCurrentParams(); return; } + const pendingParams = this.pendingParams; + // Unset before calling setUrlParams. + // This will ensure that if params are still supposed to be pending, they are not cleared. + this.pendingParams = undefined; - try { - // Do not react to `sync` method changes - const syncPromise = untrack(() => sync(newUrlParams)); - if (!syncPromise.then) { - lock = false; - return; - } - - void syncPromise.then( - () => (lock = false), - () => (lock = false), - ); - } catch { - lock = false; - } - }); + this.setUrlParams(pendingParams); + } + + /** + * Parses the current params again once the store is ready after a change in its dependencies. + * Goes around `setUrlParams` since the params themselves have not changed. + * Parts the store can no longer represent, like a filter on a dropped dimension, are removed, + * and `stateChanged` reports the removal. + */ + private reapplyCurrentParams() { + if (!this.searchParams) return; + + this.searchParams = this.store.normalizeParams( + copySubsetParams(this.searchParams, this.store.paramKeys), + ); + this.store.setUrlParams(this.searchParams); + this.stateChanged(); + } + + private maybeNotifyStateChange() { + const urlParams = new URLSearchParams(); + this.store.applyFilterToParams(urlParams); + + if (this.searchParams?.toString() === urlParams.toString()) return; + this.searchParams = urlParams; + this.events.emit("internal-change", this.searchParams); + this.events.emit("change", this.searchParams); + } } diff --git a/web-common/src/lib/url-utils.ts b/web-common/src/lib/url-utils.ts index d2f9e205cc1f..bc28f7c1f1af 100644 --- a/web-common/src/lib/url-utils.ts +++ b/web-common/src/lib/url-utils.ts @@ -33,13 +33,11 @@ export function copyWithAdditionalArguments( return newUrl; } -export function unorderedParamsAreEqual( - src: URLSearchParams, - tar: URLSearchParams, -) { - if (src.size !== tar.size) return false; - for (const [key, value] of src) { - if (value !== tar.get(key)) return false; +export function copySubsetParams(src: URLSearchParams, keys: Set) { + const newParams = new URLSearchParams(); + for (const key of keys) { + if (!src.has(key)) continue; + newParams.set(key, src.get(key)!); } - return true; + return newParams; }