From ecc5ecc47256a818e99ceed7d532d21e922ef4b4 Mon Sep 17 00:00:00 2001 From: Nishant Bangarwa Date: Wed, 7 Oct 2026 00:14:23 +0530 Subject: [PATCH] fix: pinned and required filter chips missing on canvas load Since #9974 a canvas applies its metrics views and then its YAML config, both from the same ResolveCanvas response. When the resource list has already landed, applying the names marks the specs ready synchronously, which replays the queued URL params and builds the chips before the pinned and required filters are known. Nothing re-parsed afterwards, so the chips never showed. YAMLConfigProvider now emits an update event and ExpressionFilterManager adds the missing required and pinned chips when it fires. Chips that already exist are left untouched. --- .../filters/ExpressionFilterManager.spec.ts | 75 +++++++++++++++++++ .../filters/ExpressionFilterManager.svelte.ts | 12 ++- .../filters/JoinerFilterManager.svelte.ts | 33 ++++++++ .../providers/YAMLConfigProvider.svelte.ts | 13 ++++ 4 files changed, 129 insertions(+), 4 deletions(-) diff --git a/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts b/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts index 86107a232814..b8ad1ec6d99d 100644 --- a/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts +++ b/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts @@ -666,6 +666,81 @@ describe("metrics view names change", () => { }); }); +describe("yaml config change", () => { + // A canvas applies its metrics views and then its config, both from the same ResolveCanvas + // response. When the metrics view specs are already loaded, applying the names parses the + // queued params right away, before the config is known. + it("adds required and pinned chips when the config lands after the params", async () => { + const provider = await createTestMetricsViewsProvider([ + AD_BIDS_METRICS_NAME, + ]); + provider.value.setMetricsViewNames([]); + const yamlConfigProvider = new YAMLConfigProvider(); + const { value: filterManager, destroy } = createInEffectRoot( + () => new ExpressionFilterManager(provider.value, yamlConfigProvider), + ); + cleanups.push(() => { + destroy(); + provider.value.cleanup(); + provider.destroy(); + }); + expect(filterManager.ready).toBe(false); + + // Queued until the specs are ready, which they are as soon as the names are applied. + filterManager.storeSync.setUrlParams(new URLSearchParams()); + provider.value.setMetricsViewNames([AD_BIDS_METRICS_NAME]); + expect(filterManager.ready).toBe(true); + expect(filterManager.sortedFilterManagers.dimensions).toEqual([]); + + yamlConfigProvider.update( + {}, + [AD_BIDS_PUBLISHER_DIMENSION, AD_BIDS_IMPRESSIONS_MEASURE], + [AD_BIDS_DOMAIN_DIMENSION], + ); + + expect(names(filterManager.sortedFilterManagers.dimensions)).toEqual([ + AD_BIDS_DOMAIN_DIMENSION, + AD_BIDS_PUBLISHER_DIMENSION, + ]); + expect(names(filterManager.sortedFilterManagers.measures)).toEqual([ + AD_BIDS_IMPRESSIONS_MEASURE, + ]); + expect(exprOf(filterManager, AD_BIDS_METRICS_NAME)).toBeUndefined(); + }); + + it("keeps the chips it has when the config is applied again", () => { + const yamlConfigProvider = new YAMLConfigProvider(); + yamlConfigProvider.update({}, [AD_BIDS_PUBLISHER_DIMENSION], []); + const filterManager = createFilterManager(yamlConfigProvider); + filterManager.storeSync.setUrlParams( + perMetricsViewParams({ + [AD_BIDS_METRICS_NAME]: `${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`, + }), + ); + const chipsBefore = filterManager.sortedFilterManagers.dimensions; + expect(names(chipsBefore)).toEqual([ + AD_BIDS_PUBLISHER_DIMENSION, + AD_BIDS_DOMAIN_DIMENSION, + ]); + + yamlConfigProvider.update({}, [AD_BIDS_PUBLISHER_DIMENSION], []); + + const chipsAfter = filterManager.sortedFilterManagers.dimensions; + expect(chipsAfter.length).toBe(chipsBefore.length); + chipsAfter.forEach((chip, i) => expect(chip).toBe(chipsBefore[i])); + }); + + it("skips pinned filters the metrics views do not define", () => { + const yamlConfigProvider = new YAMLConfigProvider(); + const filterManager = createFilterManager(yamlConfigProvider); + + yamlConfigProvider.update({}, ["not_a_field"], []); + + expect(filterManager.sortedFilterManagers.dimensions).toEqual([]); + expect(filterManager.sortedFilterManagers.measures).toEqual([]); + }); +}); + describe("applyFilterToParams", () => { it("writes a param per metrics view and drops the legacy singular one", () => { const filterManager = createFilterManager(); diff --git a/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts b/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts index fd0a4682cf85..a10a32c2a4e1 100644 --- a/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts +++ b/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts @@ -66,7 +66,7 @@ export class ExpressionFilterManager implements UrlParamsStore { public paramKeys = new Set([ExploreStateURLParams.Filters]); public readonly storeSync: UrlParamsChangeTracker; - // Unsubscribes from `metricsViewsProvider`, which can outlive this manager. See `cleanup`. + // Unsubscribes from the providers, which can outlive this manager. See `cleanup`. private readonly unsubscribers: (() => void)[]; public constructor( @@ -100,7 +100,11 @@ export class ExpressionFilterManager implements UrlParamsStore { this.ready = true; this.events.emit("ready"); }); - this.unsubscribers = [unsubUpdate, unsubSpecsLoaded]; + // The config can land after the params were parsed. See `addRequiredAndPinnedFilters`. + const unsubYamlConfig = yamlConfigProvider.on("update", () => + this.topLevelJoiner.addRequiredAndPinnedFilters(), + ); + this.unsubscribers = [unsubUpdate, unsubSpecsLoaded, unsubYamlConfig]; this.topLevelJoiner = $state( JoinerFilterManager.parse( @@ -157,8 +161,8 @@ export class ExpressionFilterManager implements UrlParamsStore { } /** - * Removes the listeners on `metricsViewsProvider`. - * Call this when the manager is discarded before the provider, e.g. a temporary `clone`. + * Removes the listeners on `metricsViewsProvider` and `yamlConfigProvider`. + * Call this when the manager is discarded before the providers, e.g. a temporary `clone`. */ public cleanup() { this.unsubscribers.forEach((unsub) => unsub()); diff --git a/web-common/src/features/dashboards/filters/JoinerFilterManager.svelte.ts b/web-common/src/features/dashboards/filters/JoinerFilterManager.svelte.ts index d378cce10027..8880ef69d70b 100644 --- a/web-common/src/features/dashboards/filters/JoinerFilterManager.svelte.ts +++ b/web-common/src/features/dashboards/filters/JoinerFilterManager.svelte.ts @@ -268,6 +268,39 @@ export class JoinerFilterManager { }; } + /** + * Adds a chip for each required and pinned filter that does not have one yet. + * `parse` only adds the filters the YAML config held at the time, and the config can land after + * the params: a canvas applies its metrics views before its config, both from the same spec, + * and when the metrics view specs are already loaded the queued params are parsed in between. + */ + public addRequiredAndPinnedFilters() { + if (!this.yamlConfigProvider) return; + + Object.keys(this.yamlConfigProvider.requiredFilters) + .concat(Object.keys(this.yamlConfigProvider.pinnedFilters)) + .forEach((name) => { + if (this.managerLookup[name]) return; + + const dimensionManager = DimensionFilterManager.createForMetricsViews( + this.metricsViewsProvider, + name, + { events: this.events }, + ); + if (dimensionManager) { + this.maybeAddDimensionFilter(dimensionManager); + return; + } + + const measureManager = MeasureFilterManager.createForMetricsViews( + this.metricsViewsProvider, + name, + { events: this.events }, + ); + if (measureManager) this.maybeAddMeasureFilter(measureManager); + }); + } + public removeEmptyManagers() { this.managers = { ...this.managers, diff --git a/web-common/src/features/dashboards/providers/YAMLConfigProvider.svelte.ts b/web-common/src/features/dashboards/providers/YAMLConfigProvider.svelte.ts index 019ab3966220..4530fa61e1d6 100644 --- a/web-common/src/features/dashboards/providers/YAMLConfigProvider.svelte.ts +++ b/web-common/src/features/dashboards/providers/YAMLConfigProvider.svelte.ts @@ -1,4 +1,10 @@ import { type V1Expression } from "@rilldata/web-common/runtime-client"; +import { EventEmitter } from "@rilldata/web-common/lib/event-emitter.ts"; + +type YAMLConfigProviderEvents = { + /** Fired after `update` has applied the config from a spec. */ + update: void; +}; /** * A provider for YAML only configuration. These are only mutable during yaml editing. @@ -13,6 +19,11 @@ export class YAMLConfigProvider { public cleanup: (() => void) | undefined = undefined; + private events = new EventEmitter(); + public readonly on = this.events.on.bind( + this.events, + ) as typeof this.events.on; + public update( defaultFilters: Record, pinnedFilters: string[], @@ -31,6 +42,8 @@ export class YAMLConfigProvider { ); this.requiredFilters = { ...requiredFiltersRec }; this.specRequiredFilters = { ...requiredFiltersRec }; + + this.events.emit("update"); } public setEditable(newEditable: boolean) {