From 16bece51e2ed4bfe9ed0d06aa53f542719ff4862 Mon Sep 17 00:00:00 2001 From: Nishant Bangarwa Date: Tue, 6 Oct 2026 23:11:53 +0530 Subject: [PATCH] fix: advanced filters could not be cleared from the filter bar A filter the chips cannot show (a top level OR, an operator without a chip mode, or the same dimension filtered twice) rendered only the read only "Advanced (BETA)" pill. The unified filter bar kept the Clear filters button inside the chip branch, the pill had no remove control, and `ExpressionFilterManager.clear()` never reset `isComplexFilter`. The explore session store then restored the filter on every reload of an empty url, so the only way out was clearing sessionStorage by hand. The pill gets a remove button, Clear filters renders for advanced filters in the filter bar and the canvas inspector, and clearing resets the complex flag. --- .../dashboards/filters/AdvancedFilter.svelte | 30 +++++++++- .../filters/CanvasExpressionFilters.spec.ts | 2 + .../filters/ExploreExpressionFilters.spec.ts | 2 + .../filters/ExpressionFilterManager.spec.ts | 20 +++++++ .../filters/ExpressionFilterManager.svelte.ts | 3 + .../filters/ExpressionFilters.svelte | 21 ++++--- .../StandaloneExpressionFilters.spec.ts | 2 + .../filters/VerticalExpressionFilters.svelte | 7 ++- .../test/CanvasExpressionFiltersTest.svelte | 24 ++++---- .../test/ExploreExpressionFiltersTest.svelte | 24 ++++---- .../StandaloneExpressionFiltersTest.svelte | 30 ++++++---- .../filters/test/expression-filters-suite.ts | 56 +++++++++++++++++++ .../filters/test/filter-test-utils.ts | 16 ++++++ .../test/standalone-filters-variant.ts | 3 +- web-common/src/lib/i18n/messages/en.json | 1 + web-common/src/lib/i18n/messages/es.json | 1 + .../tests/explores/advanced-filter.spec.ts | 34 +++++++++++ 17 files changed, 232 insertions(+), 44 deletions(-) create mode 100644 web-local/tests/explores/advanced-filter.spec.ts diff --git a/web-common/src/features/dashboards/filters/AdvancedFilter.svelte b/web-common/src/features/dashboards/filters/AdvancedFilter.svelte index ab62d0807cf0..71a7563f037a 100644 --- a/web-common/src/features/dashboards/filters/AdvancedFilter.svelte +++ b/web-common/src/features/dashboards/filters/AdvancedFilter.svelte @@ -1,17 +1,41 @@
+ {#if onRemove} + + + {/if} {m.filter_advanced_beta()} diff --git a/web-common/src/features/dashboards/filters/CanvasExpressionFilters.spec.ts b/web-common/src/features/dashboards/filters/CanvasExpressionFilters.spec.ts index 43fc4b6af0c2..cf9b6052606c 100644 --- a/web-common/src/features/dashboards/filters/CanvasExpressionFilters.spec.ts +++ b/web-common/src/features/dashboards/filters/CanvasExpressionFilters.spec.ts @@ -1,5 +1,6 @@ import { useCanvasFiltersVariant } from "@rilldata/web-common/features/dashboards/filters/test/canvas-filters-variant"; import { + testAdvancedFilters, testDimensionFilters, testMeasureFilters, } from "@rilldata/web-common/features/dashboards/filters/test/expression-filters-suite"; @@ -71,6 +72,7 @@ describe("CanvasExpressionFilters", () => { testDimensionFilters(variant); testMeasureFilters(variant); + testAdvancedFilters(variant); // We need to fix canvas navigation before enabling this. // Check CanvasInitialization.svelte for more details. // testURLNavigationFlows(variant); diff --git a/web-common/src/features/dashboards/filters/ExploreExpressionFilters.spec.ts b/web-common/src/features/dashboards/filters/ExploreExpressionFilters.spec.ts index 5c4d0eb894b5..3b91ef010ebd 100644 --- a/web-common/src/features/dashboards/filters/ExploreExpressionFilters.spec.ts +++ b/web-common/src/features/dashboards/filters/ExploreExpressionFilters.spec.ts @@ -1,5 +1,6 @@ import { useExploreFiltersVariant } from "@rilldata/web-common/features/dashboards/filters/test/explore-filters-variant"; import { + testAdvancedFilters, testDimensionFilters, testMeasureFilters, testUrlLengthLimit, @@ -75,4 +76,5 @@ describe("ExploreExpressionFilters", () => { testUrlLengthLimit(variant); testMeasureFilters(variant); testURLNavigationFlows(variant); + testAdvancedFilters(variant); }); diff --git a/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts b/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts index 86107a232814..dc01b02137f1 100644 --- a/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts +++ b/web-common/src/features/dashboards/filters/ExpressionFilterManager.spec.ts @@ -1382,6 +1382,26 @@ describe("clear", () => { expect(filterManager.sortedFilterManagers.measures).toEqual([]); }); + it("resets the complex flag", () => { + const filterManager = createFilterManager(); + + // A top level OR has no chip, so the bar shows the read only advanced filter. + filterManager.storeSync.setUrlParams( + perMetricsViewParams({ + [AD_BIDS_METRICS_NAME]: `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Google') OR ${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`, + }), + ); + expect(filterManager.isComplexFilter).toBe(true); + + filterManager.clear(); + + // The navigation after a clear does not parse the params again, so `clear` resets the flag itself. + expect(filterManager.isComplexFilter).toBe(false); + expect(filterManager.topLevelJoiner.expr).toEqual({}); + expect(filterManager.sortedFilterManagers.dimensions).toEqual([]); + expect(filterManager.sortedFilterManagers.measures).toEqual([]); + }); + it("keeps the required and pinned chips", () => { const yamlConfigProvider = new YAMLConfigProvider(); yamlConfigProvider.requiredFilters = { [AD_BIDS_DOMAIN_DIMENSION]: true }; diff --git a/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts b/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts index fd0a4682cf85..bc06ee9adac3 100644 --- a/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts +++ b/web-common/src/features/dashboards/filters/ExpressionFilterManager.svelte.ts @@ -316,6 +316,9 @@ export class ExpressionFilterManager implements UrlParamsStore { public clear() { this.temporaryFilterName = undefined; + // `setUrlParams` is the only other writer of this flag, + // and the navigation after a clear skips it because the params already match the cleared state. + this.isComplexFilter = false; this.topLevelJoiner.clear(); // Clearing goes through the param rather than the managers, so it has to report itself. this.events.emit("filter-changed", { source: undefined }); diff --git a/web-common/src/features/dashboards/filters/ExpressionFilters.svelte b/web-common/src/features/dashboards/filters/ExpressionFilters.svelte index fb9041af026a..c3ce6c10ef4d 100644 --- a/web-common/src/features/dashboards/filters/ExpressionFilters.svelte +++ b/web-common/src/features/dashboards/filters/ExpressionFilters.svelte @@ -89,7 +89,10 @@
{#if expressionFilterManager.isComplexFilter} {#each Object.entries(expressionFilterManager.exprByMetricsView) as [mv, expr] (mv)} - + expressionFilterManager.clear()} + /> {/each} {:else} {#if !hasFilters} @@ -135,14 +138,16 @@ {excludedDimensions} {excludedMeasures} /> + {/if} - - {#if hasClearableFilters} - - {/if} + + {#if expressionFilterManager.isComplexFilter || hasClearableFilters} + {/if}
diff --git a/web-common/src/features/dashboards/filters/StandaloneExpressionFilters.spec.ts b/web-common/src/features/dashboards/filters/StandaloneExpressionFilters.spec.ts index a43deb4212bc..c26c4cde4c03 100644 --- a/web-common/src/features/dashboards/filters/StandaloneExpressionFilters.spec.ts +++ b/web-common/src/features/dashboards/filters/StandaloneExpressionFilters.spec.ts @@ -1,4 +1,5 @@ import { + testAdvancedFilters, testDimensionFilters, testMeasureFilters, } from "@rilldata/web-common/features/dashboards/filters/test/expression-filters-suite"; @@ -69,4 +70,5 @@ describe("StandaloneExpressionFilters", () => { testDimensionFilters(variant); testMeasureFilters(variant); + testAdvancedFilters(variant); }); diff --git a/web-common/src/features/dashboards/filters/VerticalExpressionFilters.svelte b/web-common/src/features/dashboards/filters/VerticalExpressionFilters.svelte index a707df4da9c6..19dc1968c120 100644 --- a/web-common/src/features/dashboards/filters/VerticalExpressionFilters.svelte +++ b/web-common/src/features/dashboards/filters/VerticalExpressionFilters.svelte @@ -54,7 +54,10 @@
{#if expressionFilterManager.isComplexFilter} {#each Object.entries(expressionFilterManager.exprByMetricsView) as [mv, expr] (mv)} - + expressionFilterManager.clear()} + /> {/each} {:else} {#each sortedDimensionManagers as dimensionManager (dimensionManager.name)} @@ -83,7 +86,7 @@
- {#if hasFilters} + {#if hasFilters || expressionFilterManager.isComplexFilter} diff --git a/web-common/src/features/dashboards/filters/test/CanvasExpressionFiltersTest.svelte b/web-common/src/features/dashboards/filters/test/CanvasExpressionFiltersTest.svelte index 78c5dffdc292..91bc82cad5f2 100644 --- a/web-common/src/features/dashboards/filters/test/CanvasExpressionFiltersTest.svelte +++ b/web-common/src/features/dashboards/filters/test/CanvasExpressionFiltersTest.svelte @@ -1,4 +1,5 @@ - - - -
Dashboard loaded!
-
-
+ + + + + +
Dashboard loaded!
+
+
+
diff --git a/web-common/src/features/dashboards/filters/test/ExploreExpressionFiltersTest.svelte b/web-common/src/features/dashboards/filters/test/ExploreExpressionFiltersTest.svelte index 508c8ebb1ca8..c3f95156ba09 100644 --- a/web-common/src/features/dashboards/filters/test/ExploreExpressionFiltersTest.svelte +++ b/web-common/src/features/dashboards/filters/test/ExploreExpressionFiltersTest.svelte @@ -1,4 +1,5 @@ - - - -
Dashboard loaded!
-
-
+ + + + + +
Dashboard loaded!
+
+
+
diff --git a/web-common/src/features/dashboards/filters/test/StandaloneExpressionFiltersTest.svelte b/web-common/src/features/dashboards/filters/test/StandaloneExpressionFiltersTest.svelte index 30d7ef31feff..ad736f31429d 100644 --- a/web-common/src/features/dashboards/filters/test/StandaloneExpressionFiltersTest.svelte +++ b/web-common/src/features/dashboards/filters/test/StandaloneExpressionFiltersTest.svelte @@ -1,4 +1,5 @@ - -{#if metricsViewsProvider.ready} -
Dashboard loaded!
-{/if} + + + + {#if metricsViewsProvider.ready} +
Dashboard loaded!
+ {/if} +
diff --git a/web-common/src/features/dashboards/filters/test/expression-filters-suite.ts b/web-common/src/features/dashboards/filters/test/expression-filters-suite.ts index 35990d8e07c3..6c75c2ded116 100644 --- a/web-common/src/features/dashboards/filters/test/expression-filters-suite.ts +++ b/web-common/src/features/dashboards/filters/test/expression-filters-suite.ts @@ -13,6 +13,7 @@ import { getSelectAllButton, isMeasureFilterFormOpen, pressEnter, + removeAdvancedFilter, removeDimensionFilter, removeMeasureFilter, selectDimensionFilterMode, @@ -22,6 +23,7 @@ import { toggleMeasureFilter, toggleSelectAll, typeInDimensionFilterSearch, + waitForAdvancedFilter, waitForDimensionFilterResultCount, waitForDimensionFilterResults, waitForEmptyFilters, @@ -34,8 +36,10 @@ import { createInExpression, createLikeExpression, createSubQueryExpression, + getAllIdentifiers, } from "@rilldata/web-common/features/dashboards/stores/filter-utils"; import { + AD_BIDS_DOMAIN_DIMENSION, AD_BIDS_IMPRESSIONS_MEASURE, AD_BIDS_PUBLISHER_DIMENSION, } from "@rilldata/web-common/features/dashboards/stores/test-data/data"; @@ -1143,6 +1147,58 @@ export function testURLNavigationFlows(variant: ExpressionFiltersVariant) { }); } +/** + * A filter the chips cannot show falls back to the read only advanced pill. + * The pill cannot be edited, so clearing is the only way out of it. + */ +export function testAdvancedFilters(variant: ExpressionFiltersVariant) { + const { + initialUrlSearch, + urlSearchWithFilter, + assertWhereFilter, + assertUrlSearch, + } = variantAssertions(variant); + + describe("Advanced filters", () => { + // A top level OR has no chip of its own. + const orFilter = `${AD_BIDS_PUBLISHER_DIMENSION} IN ('Facebook') OR ${AD_BIDS_DOMAIN_DIMENSION} IN ('google.com')`; + + it("Should clear an advanced filter with the clear button", async () => { + await variant.render(urlSearchWithFilter(orFilter)); + await waitForAdvancedFilter(orFilter); + + // The filter reaches the consumer even though no chip can show it. + // Only the identifiers are checked, since the explore state holds the OR as is while a + // standalone bar wraps it in an AND. + expect( + getAllIdentifiers(variant.expressionFilterManager.getWhereFilter()), + ).toEqual([AD_BIDS_PUBLISHER_DIMENSION, AD_BIDS_DOMAIN_DIMENSION]); + // Nothing can be added next to the pill. + expect( + screen.queryByLabelText("Add filter button"), + ).not.toBeInTheDocument(); + + await clearFilters(); + + expect(screen.queryByText("Advanced (BETA)")).not.toBeInTheDocument(); + expect(screen.getByLabelText("Add filter button")).toBeVisible(); + assertWhereFilter(createAndExpression([])); + assertUrlSearch(initialUrlSearch); + }); + + it("Should clear an advanced filter from its pill", async () => { + await variant.render(urlSearchWithFilter(orFilter)); + await waitForAdvancedFilter(orFilter); + + await removeAdvancedFilter(); + + await waitForEmptyFilters(); + assertWhereFilter(createAndExpression([])); + assertUrlSearch(initialUrlSearch); + }); + }); +} + /** The expression a measure filter on `impressions`, split by `publisher`, produces. */ export function measureFilterExpression(operation: V1Operation, value: number) { return createSubQueryExpression( diff --git a/web-common/src/features/dashboards/filters/test/filter-test-utils.ts b/web-common/src/features/dashboards/filters/test/filter-test-utils.ts index 6dea2a28298e..00e4eacb8ece 100644 --- a/web-common/src/features/dashboards/filters/test/filter-test-utils.ts +++ b/web-common/src/features/dashboards/filters/test/filter-test-utils.ts @@ -311,6 +311,22 @@ export async function clearFilters() { await waitForEmptyFilters(); } +/** Waits for the advanced filter pill, which shows the filter param no chip can represent. */ +export async function waitForAdvancedFilter(filter: string) { + await waitFor(() => + expect(screen.getByText("Advanced (BETA)")).toBeVisible(), + ); + // The pill writes a nested expression with its parentheses, so match on the text within them. + expect(screen.getByText("Advanced (BETA)").parentElement).toHaveTextContent( + filter, + ); +} + +/** Removes the advanced filter pill, which clears the whole filter. */ +export async function removeAdvancedFilter() { + await act(() => screen.getByRole("button", { name: "Remove" }).click()); +} + /** * Whether the measure filter form is open. * diff --git a/web-common/src/features/dashboards/filters/test/standalone-filters-variant.ts b/web-common/src/features/dashboards/filters/test/standalone-filters-variant.ts index 971205ef1c44..691d3ae8c1c0 100644 --- a/web-common/src/features/dashboards/filters/test/standalone-filters-variant.ts +++ b/web-common/src/features/dashboards/filters/test/standalone-filters-variant.ts @@ -73,10 +73,11 @@ export function useStandaloneFiltersVariant( getInListDimensions: () => expressionFilterManager!.inList, }, - render: async () => { + render: async (initUrlSearch?: string) => { render(StandaloneExpressionFiltersTest, { props: { metricsViewNames: [AD_BIDS_METRICS_NAME], + initUrlSearch, onManagerCreated: (manager: ExpressionFilterManager) => (expressionFilterManager = manager), }, diff --git a/web-common/src/lib/i18n/messages/en.json b/web-common/src/lib/i18n/messages/en.json index 6d6225bb7448..d601fcb087ac 100644 --- a/web-common/src/lib/i18n/messages/en.json +++ b/web-common/src/lib/i18n/messages/en.json @@ -1292,6 +1292,7 @@ "field_list_measures": "MEASURES", "field_list_time": "TIME", "filter_advanced_beta": "Advanced (BETA)", + "filter_advanced_remove": "Remove", "filter_advanced_warning": "Advanced filters are a bleeding edge feature! There may be bugs.", "filter_converted_to_select": "Converted filter type to Select", "filter_dimension": "dimension", diff --git a/web-common/src/lib/i18n/messages/es.json b/web-common/src/lib/i18n/messages/es.json index d49b726ac846..b3583ba8683e 100644 --- a/web-common/src/lib/i18n/messages/es.json +++ b/web-common/src/lib/i18n/messages/es.json @@ -1301,6 +1301,7 @@ "field_list_measures": "MEDIDAS", "field_list_time": "TIEMPO", "filter_advanced_beta": "Avanzado (BETA)", + "filter_advanced_remove": "Eliminar", "filter_advanced_warning": "Los filtros avanzados son una funcionalidad experimental. Puede haber errores.", "filter_converted_to_select": "Tipo de filtro convertido a Seleccionar", "filter_dimension": "dimensión", diff --git a/web-local/tests/explores/advanced-filter.spec.ts b/web-local/tests/explores/advanced-filter.spec.ts new file mode 100644 index 000000000000..ad2d98f6e25f --- /dev/null +++ b/web-local/tests/explores/advanced-filter.spec.ts @@ -0,0 +1,34 @@ +import { expect } from "@playwright/test"; +import { test } from "../setup/base"; +import { waitForReconciliation } from "../utils/wait-for-reconciliation.ts"; + +// A filter the chips cannot show, such as a top level OR, falls back to the read only +// "Advanced (BETA)" pill. Clearing it has to work from the UI, and the cleared state has to be +// what the session restores afterwards, since an empty explore url loads the last state of the tab. +test.describe("advanced filters", () => { + test.use({ project: "AdBids" }); + + test("clears an advanced filter from a shared url and keeps it cleared on reload", async ({ + page, + }) => { + const { origin } = new URL(page.url()); + await waitForReconciliation(page); + + const filter = "publisher IN ('Facebook') OR domain IN ('google.com')"; + await page.goto( + `${origin}/explore/AdBids_metrics_explore?f=${encodeURIComponent(filter)}`, + ); + await expect(page.getByText("Advanced (BETA)")).toBeVisible(); + await expect(page.getByRole("button", { name: "Remove" })).toBeVisible(); + await expect(page.getByLabel("Add filter button")).toBeHidden(); + + await page.getByRole("button", { name: "Clear filters" }).click(); + await expect(page.getByText("No filters selected")).toBeVisible(); + await expect(page.getByLabel("Add filter button")).toBeVisible(); + await page.waitForURL((url) => !url.searchParams.has("f")); + + await page.reload(); + await expect(page.getByText("No filters selected")).toBeVisible(); + expect(new URL(page.url()).searchParams.has("f")).toBe(false); + }); +});