diff --git a/.changeset/five-frogs-dream.md b/.changeset/five-frogs-dream.md new file mode 100644 index 0000000000..49e30b33f5 --- /dev/null +++ b/.changeset/five-frogs-dream.md @@ -0,0 +1,5 @@ +--- +"@hyperdx/app": patch +--- + +feat: preserve compatible filters when switching sources diff --git a/packages/app/src/DBSearchPage.tsx b/packages/app/src/DBSearchPage.tsx index 7ed4f2d3a7..92fe91474c 100644 --- a/packages/app/src/DBSearchPage.tsx +++ b/packages/app/src/DBSearchPage.tsx @@ -25,7 +25,10 @@ import { import { useForm, useWatch } from 'react-hook-form'; import { z } from 'zod'; import { zodResolver } from '@hookform/resolvers/zod'; -import { ClickHouseQueryError } from '@hyperdx/common-utils/dist/clickhouse'; +import { + ClickHouseQueryError, + ColumnMeta, +} from '@hyperdx/common-utils/dist/clickhouse'; import { tcFromSource } from '@hyperdx/common-utils/dist/core/metadata'; import { buildSearchChartConfig } from '@hyperdx/common-utils/dist/core/searchChartConfig'; import { @@ -129,8 +132,9 @@ import { getRelativeTimeOptionLabel, LIVE_TAIL_DURATION_MS, } from './components/TimePicker/utils'; -import { useTableMetadata } from './hooks/useMetadata'; +import { useColumns, useTableMetadata } from './hooks/useMetadata'; import { useSqlSuggestions } from './hooks/useSqlSuggestions'; +import { useStableCallback } from './hooks/useStableCallback'; import { buildDirectTraceWhereClause, getDefaultDirectTraceDateRange, @@ -783,6 +787,12 @@ export function useDefaultOrderBy(sourceID: string | undefined | null) { }, [source, tableMetadata]); } +function formatDroppedFiltersMessage(count: number): string { + const noun = count === 1 ? 'filter' : 'filters'; + const verb = count === 1 ? 'was' : 'were'; + return `${count} ${noun} didn't apply to this source and ${verb} removed.`; +} + // This is outside as it needs to be a stable reference const queryStateMap = { source: parseAsString, @@ -1070,6 +1080,23 @@ export function DBSearchPage() { defaultValue: searchedConfig.source ?? undefined, }); const prevSourceRef = useRef(watchedSource); + // Set when the user switches sources via the dropdown. The follow-up + // effect waits for the new source's columns to load and then drops any + // sidebar filters that don't apply to the new schema. + const pendingFilterReconcileRef = useRef(null); + + const watchedSourceObj = useMemo( + () => inputSourceObjs?.find(s => s.id === watchedSource), + [inputSourceObjs, watchedSource], + ); + const { data: watchedSourceColumns } = useColumns( + { + databaseName: watchedSourceObj?.from?.databaseName ?? '', + tableName: watchedSourceObj?.from?.tableName ?? '', + connectionId: watchedSourceObj?.connection ?? '', + }, + { enabled: !!watchedSourceObj }, + ); useEffect(() => { // If the user changes the source dropdown, reset the select and orderby fields @@ -1087,14 +1114,19 @@ export function DBSearchPage() { if (savedSearchId == null || savedSearch?.source !== watchedSource) { setValue('select', ''); setValue('orderBy', ''); - // Clear all search filters only when switching to a different source - searchFilters.clearAllFilters(); + // Defer filter clearing: wait until the new source's columns load, + // then keep filters whose root column exists on the new schema. + pendingFilterReconcileRef.current = watchedSource ?? null; // If the user is in a saved search, prefer the saved search's select/orderBy if available } else { setValue('select', savedSearch?.select ?? ''); setValue('orderBy', savedSearch?.orderBy ?? ''); // Don't clear filters - we're loading from saved search } + // Push the new source to URL/searchedConfig so the chart re-queries. + // Debounced so a later filter reconcile (which also submits) collapses + // into a single run. + debouncedSubmit(); } } }, [ @@ -1103,10 +1135,34 @@ export function DBSearchPage() { savedSearch, savedSearchId, inputSourceObjs, - searchFilters, setLastSelectedSourceId, + debouncedSubmit, ]); + const retainCompatibleFilters = useStableCallback((columns: ColumnMeta[]) => { + pendingFilterReconcileRef.current = null; + + const allowed = new Set(columns.map(c => c.name)); + + const dropped = searchFilters.retainFiltersByColumns(allowed); + + if (dropped.length > 0) { + notifications.show({ + color: 'yellow', + message: formatDroppedFiltersMessage(dropped.length), + }); + } + }); + + useEffect(() => { + if ( + pendingFilterReconcileRef.current === watchedSource && + watchedSourceColumns + ) { + retainCompatibleFilters(watchedSourceColumns); + } + }, [watchedSource, watchedSourceColumns, retainCompatibleFilters]); + const onTableScroll = useCallback( (scrollTop: number) => { // If the user scrolls a bit down, kick out of live mode diff --git a/packages/app/src/__tests__/DBSearchPage.directTrace.test.tsx b/packages/app/src/__tests__/DBSearchPage.directTrace.test.tsx index b88d8bf800..ac3b7e5c63 100644 --- a/packages/app/src/__tests__/DBSearchPage.directTrace.test.tsx +++ b/packages/app/src/__tests__/DBSearchPage.directTrace.test.tsx @@ -132,6 +132,10 @@ jest.mock('../hooks/useMetadata', () => ({ data: { sorting_key: 'Timestamp' }, isLoading: false, }), + useColumns: () => ({ + data: undefined, + isLoading: false, + }), })); jest.mock('../hooks/useSqlSuggestions', () => ({ diff --git a/packages/app/src/__tests__/searchFilters.test.ts b/packages/app/src/__tests__/searchFilters.test.ts index 92c82bd8b2..1d30fc7314 100644 --- a/packages/app/src/__tests__/searchFilters.test.ts +++ b/packages/app/src/__tests__/searchFilters.test.ts @@ -1002,6 +1002,159 @@ describe('searchFilters', () => { // No migration needed — onFilterChange should not be called expect(onFilterChangeNoMigrate).not.toHaveBeenCalled(); }); + + describe('retainFiltersByColumns', () => { + it('returns [] and does not touch URL when filter state is empty', () => { + const onFilterChangeLocal = jest.fn(); + const { result } = renderHook(() => + useSearchPageFilterState({ + searchQuery: [], + onFilterChange: onFilterChangeLocal, + }), + ); + + let dropped: string[] = ['unset']; + act(() => { + dropped = result.current.retainFiltersByColumns( + new Set(['ServiceName']), + ); + }); + + expect(dropped).toEqual([]); + expect(onFilterChangeLocal).not.toHaveBeenCalled(); + }); + + it('keeps filters whose root column exists on the new source', () => { + const onFilterChangeLocal = jest.fn(); + const { result } = renderHook(() => + useSearchPageFilterState({ + searchQuery: [ + { type: 'lucene', condition: 'ServiceName:"app"' }, + { type: 'lucene', condition: 'SeverityText:"error"' }, + ], + onFilterChange: onFilterChangeLocal, + }), + ); + + let dropped: string[] = ['unset']; + act(() => { + dropped = result.current.retainFiltersByColumns( + new Set(['ServiceName', 'SeverityText', 'Timestamp']), + ); + }); + + expect(dropped).toEqual([]); + // Nothing dropped → no URL update fires. + expect(onFilterChangeLocal).not.toHaveBeenCalled(); + }); + + it('keeps nested JSON/Map keys when the root column exists', () => { + const onFilterChangeLocal = jest.fn(); + const { result } = renderHook(() => + useSearchPageFilterState({ + searchQuery: [ + { + type: 'lucene', + condition: 'LogAttributes.user:"123"', + }, + ], + onFilterChange: onFilterChangeLocal, + }), + ); + + let dropped: string[] = ['unset']; + act(() => { + dropped = result.current.retainFiltersByColumns( + new Set(['LogAttributes']), + ); + }); + + expect(dropped).toEqual([]); + expect(onFilterChangeLocal).not.toHaveBeenCalled(); + }); + + it('drops filters whose root column is missing and returns their keys', () => { + const onFilterChangeLocal = jest.fn(); + const { result } = renderHook(() => + useSearchPageFilterState({ + searchQuery: [ + { type: 'lucene', condition: 'OldColumn:"x"' }, + { type: 'lucene', condition: 'AnotherGone:"y"' }, + ], + onFilterChange: onFilterChangeLocal, + }), + ); + + let dropped: string[] = []; + act(() => { + dropped = result.current.retainFiltersByColumns( + new Set(['ServiceName']), + ); + }); + + expect(dropped.sort()).toEqual(['AnotherGone', 'OldColumn']); + expect(onFilterChangeLocal).toHaveBeenLastCalledWith([]); + }); + + it('keeps matching filters and drops the rest in mixed input', () => { + const onFilterChangeLocal = jest.fn(); + const { result } = renderHook(() => + useSearchPageFilterState({ + searchQuery: [ + { type: 'lucene', condition: 'ServiceName:"app"' }, + { type: 'lucene', condition: 'Body:"oops"' }, + ], + onFilterChange: onFilterChangeLocal, + }), + ); + + let dropped: string[] = []; + act(() => { + dropped = result.current.retainFiltersByColumns( + new Set(['ServiceName', 'Timestamp']), + ); + }); + + expect(dropped).toEqual(['Body']); + expect(onFilterChangeLocal).toHaveBeenLastCalledWith([ + { type: 'lucene', condition: 'ServiceName:"app"' }, + ]); + }); + + it('preserves passthrough filters when reconciling', () => { + const onFilterChangeLocal = jest.fn(); + const warnSpy = jest + .spyOn(console, 'warn') + .mockImplementation(() => {}); + // Unparseable Lucene (mismatched parens) is preserved by parseQuery + // as a passthrough filter. + const { result } = renderHook(() => + useSearchPageFilterState({ + searchQuery: [ + { type: 'lucene', condition: 'ServiceName:"app"' }, + { type: 'lucene', condition: 'Body:"oops"' }, + { type: 'lucene', condition: '((((' }, + ], + onFilterChange: onFilterChangeLocal, + }), + ); + + let dropped: string[] = []; + act(() => { + dropped = result.current.retainFiltersByColumns( + new Set(['ServiceName']), + ); + }); + + expect(dropped).toEqual(['Body']); + // Passthrough filter rides along after the kept filters. + expect(onFilterChangeLocal).toHaveBeenLastCalledWith([ + { type: 'lucene', condition: 'ServiceName:"app"' }, + { type: 'lucene', condition: '((((' }, + ]); + warnSpy.mockRestore(); + }); + }); }); describe('filters use direct_read optimization', () => { diff --git a/packages/app/src/components/__tests__/ActiveFilterPills.test.tsx b/packages/app/src/components/__tests__/ActiveFilterPills.test.tsx index cbe4efd43d..99dac06164 100644 --- a/packages/app/src/components/__tests__/ActiveFilterPills.test.tsx +++ b/packages/app/src/components/__tests__/ActiveFilterPills.test.tsx @@ -14,6 +14,7 @@ function makeSearchFilters( setFilterRange: jest.fn(), clearFilter: jest.fn(), clearAllFilters: jest.fn(), + retainFiltersByColumns: jest.fn(() => []), }; } diff --git a/packages/app/src/searchFilters.tsx b/packages/app/src/searchFilters.tsx index cc35e484fd..23ec18ba42 100644 --- a/packages/app/src/searchFilters.tsx +++ b/packages/app/src/searchFilters.tsx @@ -525,6 +525,34 @@ export const useSearchPageFilterState = ({ updateFilterQuery({}); }, [updateFilterQuery]); + // Keep only filters whose root column appears in `allowedColumnNames`. + // Returns the keys of the filters that were dropped so callers can surface + // a notice when filter state was thrown away (e.g. on source change). + const retainFiltersByColumns = useCallback( + (allowedColumnNames: Set): string[] => { + const dropped: string[] = []; + const kept: FilterState = {}; + for (const [key, value] of Object.entries(filters)) { + // Filter keys are dot-normalized — top-level columns are stored as-is, + // nested JSON/Map keys as `Root.nested.path`. An exact match handles + // the rare case of a column with dots in its name. + const dotIdx = key.indexOf('.'); + const rootColumn = dotIdx > 0 ? key.slice(0, dotIdx) : key; + if (allowedColumnNames.has(key) || allowedColumnNames.has(rootColumn)) { + kept[key] = value; + } else { + dropped.push(key); + } + } + if (dropped.length > 0) { + setFilters(kept); + updateFilterQuery(kept); + } + return dropped; + }, + [filters, updateFilterQuery], + ); + return { filters, setFilters, @@ -532,6 +560,7 @@ export const useSearchPageFilterState = ({ setFilterRange, clearFilter, clearAllFilters, + retainFiltersByColumns, }; }; diff --git a/packages/app/tests/e2e/features/search/search-filters.spec.ts b/packages/app/tests/e2e/features/search/search-filters.spec.ts index 7e5d2c3c39..a701c14230 100644 --- a/packages/app/tests/e2e/features/search/search-filters.spec.ts +++ b/packages/app/tests/e2e/features/search/search-filters.spec.ts @@ -1,5 +1,10 @@ import { SearchPage } from '../../page-objects/SearchPage'; +import { SERVICES } from '../../seed-clickhouse'; import { expect, test } from '../../utils/base-test'; +import { + DEFAULT_LOGS_SOURCE_NAME, + DEFAULT_TRACES_SOURCE_NAME, +} from '../../utils/constants'; test.describe('Search Filters', { tag: ['@search'] }, () => { let searchPage: SearchPage; @@ -104,6 +109,111 @@ test.describe('Search Filters', { tag: ['@search'] }, () => { // // Add methods to FilterComponent for show more/less // }); }); + +// HDX-4254: Preserve compatible filters when switching sources +test.describe( + 'Source switching — filter preservation', + { tag: ['@search'] }, + () => { + let searchPage: SearchPage; + + test.beforeEach(async ({ page }) => { + searchPage = new SearchPage(page); + await searchPage.goto(); + // Start every test on the logs source so the baseline is consistent + await searchPage.selectSource(DEFAULT_LOGS_SOURCE_NAME); + await searchPage.table.waitForRowsToPopulate(); + }); + + test('Compatible filter is preserved when switching to a source that shares the column — no toast', async () => { + // Pick a visible ServiceName value from the seeded candidates + // (pickVisibleFilterValues also opens the filter group) + const [serviceName] = await searchPage.filters.pickVisibleFilterValues( + 'ServiceName', + SERVICES, + 1, + ); + + // Apply the ServiceName filter + await searchPage.filters.applyFilter('ServiceName', serviceName); + const filterInput = searchPage.filters.getFilterCheckboxInput( + 'ServiceName', + serviceName, + ); + await expect(filterInput).toBeChecked(); + + // Switch to E2E Traces — ServiceName also exists on that schema + await searchPage.selectSource(DEFAULT_TRACES_SOURCE_NAME); + await searchPage.table.waitForRowsToPopulate(); + + // Re-open ServiceName group (it may collapse during source switch) + await searchPage.filters.openFilterGroup('ServiceName'); + + // The filter must still be in the URL — implementation contract for HDX-4254. + // We assert the URL rather than the sidebar checkbox because the trace source + // may render a different set of facet values (e.g. different service names), + // making a checkbox-checked assertion fragile even when the filter is correctly + // preserved in state. The URL is the authoritative source of truth here. + await expect(searchPage.page).toHaveURL(/filters=.*ServiceName/i); + + // No "didn't apply" toast should have appeared + await expect(searchPage.getDroppedFiltersToast()).toHaveCount(0); + }); + + test('Incompatible filter is dropped and toast shown when one of two filters does not exist on new source', async () => { + // Apply a ServiceName filter (shared between logs and traces) + // (pickVisibleFilterValues also opens the filter group) + const [serviceName] = await searchPage.filters.pickVisibleFilterValues( + 'ServiceName', + SERVICES, + 1, + ); + await searchPage.filters.applyFilter('ServiceName', serviceName); + + // Open and apply a SeverityText filter (logs-only column, not present on traces) + await searchPage.filters.openFilterGroup('SeverityText'); + await searchPage.filters.applyFilter('SeverityText', 'info'); + await expect( + searchPage.filters.getFilterCheckboxInput('SeverityText', 'info'), + ).toBeChecked(); + + // Switch to E2E Traces — SeverityText does not exist on that schema + await searchPage.selectSource(DEFAULT_TRACES_SOURCE_NAME); + await searchPage.table.waitForRowsToPopulate(); + + // Yellow toast must appear with "1 filter didn't apply to this source" + await expect(searchPage.getDroppedFiltersToast()).toBeVisible({ + timeout: 10000, + }); + + // Re-open ServiceName group and confirm the compatible filter was kept. + // Assert via URL rather than checkbox — the trace source's facet may show + // a different set of service name values, so the checkbox checked state is + // fragile even when the filter is correctly preserved in the URL/state. + await searchPage.filters.openFilterGroup('ServiceName'); + await expect(searchPage.page).toHaveURL(/filters=.*ServiceName/i); + }); + + test('Filter for a column that does not exist on new source is dropped and toast shown', async () => { + // Open and apply only a SeverityText filter (logs-only, not present on traces) + await searchPage.filters.openFilterGroup('SeverityText'); + await searchPage.filters.applyFilter('SeverityText', 'info'); + await expect( + searchPage.filters.getFilterCheckboxInput('SeverityText', 'info'), + ).toBeChecked(); + + // Switch to E2E Traces — SeverityText is absent from traces schema + await searchPage.selectSource(DEFAULT_TRACES_SOURCE_NAME); + await searchPage.table.waitForRowsToPopulate(); + + // Toast must appear indicating 1 filter was removed + await expect(searchPage.getDroppedFiltersToast()).toBeVisible({ + timeout: 10000, + }); + }); + }, +); + // HDX-3901: Filter parsing with special characters (=, >, <, OR) in quoted // values is tested via unit tests in searchFilters.test.ts (6 dedicated test // cases). The parseQuery / extractInClauses / containsOperatorOutsideQuotes diff --git a/packages/app/tests/e2e/page-objects/SearchPage.ts b/packages/app/tests/e2e/page-objects/SearchPage.ts index 2963ad42dc..6734ab72b9 100644 --- a/packages/app/tests/e2e/page-objects/SearchPage.ts +++ b/packages/app/tests/e2e/page-objects/SearchPage.ts @@ -295,6 +295,18 @@ export class SearchPage { await this.page.mouse.up(); } + /** + * Returns a locator for the yellow Mantine notification shown when one or + * more sidebar filters are dropped because they don't exist on the newly + * selected source's schema. + * + * The message format from DBSearchPage.tsx: + * "N filter(s) didn't apply to this source and was/were removed." + */ + getDroppedFiltersToast() { + return this.page.getByText(/filter.* didn't apply to this source/i); + } + // Getters for assertions in spec files get form() {