diff --git a/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/index.tsx b/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/index.tsx index 2b027080365..a44c7b26c71 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/index.tsx +++ b/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/index.tsx @@ -214,6 +214,7 @@ export type { CellClassParams, IMenuActionParams, IHeaderParams, + SelectionChangedEvent, SortModelItem, ValueFormatterParams, ValueGetterParams, diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx index 4b6bd9e276f..5659077765d 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx @@ -40,7 +40,7 @@ import { GridReadyEvent, GridState, CellClickedEvent, - IMenuActionParams, + SelectionChangedEvent, } from '@superset-ui/core/components/ThemedAgGridReact'; import { t } from '@apache-superset/core/translation'; import { @@ -96,8 +96,9 @@ export interface AgGridTableProps { percentMetrics: string[]; serverPageLength: number; hasServerPageLengthChanged: boolean; - handleCrossFilter: (event: CellClickedEvent | IMenuActionParams) => void; - isActiveFilterValue: (key: string, val: DataRecordValue) => boolean; + handleCellClicked: (event: CellClickedEvent) => void; + handleSelectionChanged: (event: SelectionChangedEvent) => void; + filters?: Record | null; renderTimeComparisonDropdown: () => JSX.Element | null; cleanedTotals: DataRecord; showTotals: boolean; @@ -135,8 +136,9 @@ const AgGridDataTable: FunctionComponent = memo( percentMetrics, serverPageLength, hasServerPageLengthChanged, - handleCrossFilter, - isActiveFilterValue, + handleCellClicked, + handleSelectionChanged, + filters, renderTimeComparisonDropdown, cleanedTotals, showTotals, @@ -422,6 +424,15 @@ const AgGridDataTable: FunctionComponent = memo( } }, [width]); + useEffect(() => { + if ( + (!filters || Object.keys(filters).length === 0) && + gridRef.current?.api?.getSelectedRows().length + ) { + gridRef.current.api.deselectAll(); + } + }, [filters]); + const onGridReady = (params: GridReadyEvent) => { // This will make columns fill the grid width params.api.sizeColumnsToFit(); @@ -500,7 +511,8 @@ const AgGridDataTable: FunctionComponent = memo( onColumnGroupOpened={params => params.api.sizeColumnsToFit()} rowSelection="multiple" animateRows - onCellClicked={handleCrossFilter} + onCellClicked={handleCellClicked} + onSelectionChanged={handleSelectionChanged} onFilterChanged={handleFilterChanged} onStateUpdated={handleGridStateChange} initialState={gridInitialState} @@ -592,7 +604,6 @@ const AgGridDataTable: FunctionComponent = memo( initialSortState: getInitialSortState( serverPaginationData?.sortBy || [], ), - isActiveFilterValue, lastFilteredColumn: serverPaginationData?.lastFilteredColumn, lastFilteredInputPosition: serverPaginationData?.lastFilteredInputPosition, diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx index 0673bdd483a..2695b530dfe 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx @@ -23,12 +23,12 @@ import { getTimeFormatterForGranularity, } from '@superset-ui/core'; import { GenericDataType } from '@apache-superset/core/common'; -import { useCallback, useEffect, useState, useMemo } from 'react'; +import { useCallback, useEffect, useRef, useState, useMemo } from 'react'; import { isEqual } from 'lodash'; import { CellClickedEvent, - IMenuActionParams, + SelectionChangedEvent, } from '@superset-ui/core/components/ThemedAgGridReact'; import { AgGridTableChartTransformedProps, @@ -40,7 +40,7 @@ import AgGridDataTable from './AgGridTable'; import { updateTableOwnState } from './utils/externalAPIs'; import TimeComparisonVisibility from './AgGridTable/components/TimeComparisonVisibility'; import { useColDefs } from './utils/useColDefs'; -import { getCrossFilterDataMask } from './utils/getCrossFilterDataMask'; +import { buildSelectionCrossFilterDataMask } from './utils/getCrossFilterDataMask'; import { StyledChartContainer } from './styles'; import type { FilterState } from './utils/filterStateManager'; @@ -248,7 +248,14 @@ export default function TableChart( const isActiveFilterValue = useCallback( function isActiveFilterValue(key: string, val: DataRecordValue) { - return !!filters && filters[key]?.includes(val); + if (!filters || !filters[key]) return false; + return filters[key].some(filterVal => { + if (filterVal === val) return true; + if (filterVal instanceof Date && val instanceof Date) { + return filterVal.getTime() === val.getTime(); + } + return false; + }); }, [filters], ); @@ -263,37 +270,68 @@ export default function TableChart( [timeGrain, isRawRecords], ); - const toggleFilter = useCallback( - (event: CellClickedEvent | IMenuActionParams) => { + const activeColumnRef = useRef(null); + + const handleCellClicked = useCallback( + (event: CellClickedEvent) => { + if (!emitCrossFilters || !event.column) return; + const colDef = event.column.getColDef(); + if (colDef.context?.isMetric || colDef.context?.isPercentMetric) return; + + const key = event.column.getColId(); + activeColumnRef.current = key; + + // Re-click on already-filtered single selection → untoggle + // AG Grid doesn't change selection when re-clicking the same row, + // so onSelectionChanged won't fire — handle clear directly here + const selectedNodes = event.api.getSelectedNodes(); if ( - emitCrossFilters && - event.column && - !( - event.column.getColDef().context?.isMetric || - event.column.getColDef().context?.isPercentMetric - ) + selectedNodes.length === 1 && + selectedNodes[0] === event.node && + isActiveFilterValue(key, event.value) ) { - const crossFilterProps = { - key: event.column.getColId(), - value: event.value, - filters, - timeGrain, - isActiveFilterValue, - timestampFormatter, - }; - setDataMask(getCrossFilterDataMask(crossFilterProps).dataMask); + event.node.setSelected(false); + setDataMask( + buildSelectionCrossFilterDataMask({ + key, + values: [], + timeGrain, + timestampFormatter, + }).dataMask, + ); } }, [ emitCrossFilters, - setDataMask, - filters, - timeGrain, isActiveFilterValue, + setDataMask, + timeGrain, timestampFormatter, ], ); + const handleSelectionChanged = useCallback( + (event: SelectionChangedEvent) => { + if (!emitCrossFilters || !activeColumnRef.current) return; + + const key = activeColumnRef.current; + const selectedRows = event.api.getSelectedRows(); + const values = selectedRows + .map(row => row[key] as DataRecordValue) + .filter(v => v != null); + + setDataMask( + buildSelectionCrossFilterDataMask({ + key, + values, + timeGrain, + timestampFormatter, + }).dataMask, + ); + }, + [emitCrossFilters, setDataMask, timeGrain, timestampFormatter], + ); + const handleServerPaginationChange = useCallback( (pageNumber: number, pageSize: number) => { const modifiedOwnState = { @@ -395,11 +433,12 @@ export default function TableChart( onFilterChanged={handleFilterChanged} metricColumns={metricColumns} id={slice_id} - handleCrossFilter={toggleFilter} + handleCellClicked={handleCellClicked} + handleSelectionChanged={handleSelectionChanged} + filters={filters} percentMetrics={percentMetrics} serverPageLength={serverPageLength} hasServerPageLengthChanged={hasServerPageLengthChanged} - isActiveFilterValue={isActiveFilterValue} renderTimeComparisonDropdown={ isUsingTimeComparison ? renderTimeComparisonVisibility : () => null } diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellClass.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellClass.ts index fec452ef581..534357756b9 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellClass.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellClass.ts @@ -27,15 +27,11 @@ type GetCellClassParams = CellClassParams & { const getCellClass = (params: GetCellClassParams) => { const { col, emitCrossFilters } = params; - const isActiveFilterValue = params?.context?.isActiveFilterValue; let className = ''; if (emitCrossFilters) { if (!col?.isMetric) { className += ' dt-is-filter'; } - if (isActiveFilterValue?.(col?.key, params?.value)) { - className += ' dt-is-active-filter'; - } if (col?.config?.truncateLongCells) { className += ' dt-truncate-cell'; } diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCrossFilterDataMask.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCrossFilterDataMask.ts index 6cf09e9be7e..ef404606931 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCrossFilterDataMask.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCrossFilterDataMask.ts @@ -34,6 +34,55 @@ type GetCrossFilterDataMaskProps = { timestampFormatter: (value: DataRecordValue) => string; }; +type BuildSelectionCrossFilterProps = { + key: string; + values: DataRecordValue[]; + timeGrain?: TimeGranularity; + timestampFormatter: (value: DataRecordValue) => string; +}; + +export const buildSelectionCrossFilterDataMask = ({ + key, + values, + timeGrain, + timestampFormatter, +}: BuildSelectionCrossFilterProps) => { + if (values.length === 0) { + return { + dataMask: { + extraFormData: { filters: [] }, + filterState: { label: null, value: null, filters: null }, + }, + }; + } + + const updatedFilters: DataRecordFilters = { [key]: values }; + const isTimestamp = key === DTTM_ALIAS; + const label = values + .map(v => (isTimestamp ? timestampFormatter(v) : v)) + .join(', '); + + return { + dataMask: { + extraFormData: { + filters: [ + { + col: key, + op: 'IN' as const, + val: values.map(el => (el instanceof Date ? el.getTime() : el!)), + grain: isTimestamp ? timeGrain : undefined, + }, + ], + }, + filterState: { + label, + value: [values], + filters: updatedFilters, + }, + }, + }; +}; + export const getCrossFilterDataMask = ({ key, value, diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx b/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx index ef1dbb9a998..56ed2cb5c90 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx @@ -439,11 +439,22 @@ export default function PivotTableChart(props: PivotTableProps) { [groupbyColumnsRaw, groupbyRowsRaw, setDataMask], ); + const isActiveFilterValue = useCallback( + (key: string, val: DataRecordValue) => { + if (!selectedFilters || !selectedFilters[key]) return false; + return selectedFilters[key].some((filterVal: DataRecordValue) => { + if (filterVal === val) return true; + if (filterVal instanceof Date && val instanceof Date) { + return filterVal.getTime() === val.getTime(); + } + return false; + }); + }, + [selectedFilters], + ); + const getCrossFilterDataMask = useCallback( (value: { [key: string]: string }) => { - const isActiveFilterValue = (key: string, val: DataRecordValue) => - !!selectedFilters && selectedFilters[key]?.includes(val); - if (!value) { return undefined; } @@ -500,7 +511,7 @@ export default function PivotTableChart(props: PivotTableProps) { isCurrentValueSelected: isActiveFilterValue(key, val), }; }, - [groupbyColumnsRaw, groupbyRowsRaw, selectedFilters], + [groupbyColumnsRaw, groupbyRowsRaw, isActiveFilterValue, selectedFilters], ); const toggleFilter = useCallback( @@ -521,9 +532,6 @@ export default function PivotTableChart(props: PivotTableProps) { return; } - const isActiveFilterValue = (key: string, val: DataRecordValue) => - !!selectedFilters && selectedFilters[key]?.includes(val); - const filtersCopy = { ...filters }; delete filtersCopy[METRIC_KEY]; @@ -533,16 +541,24 @@ export default function PivotTableChart(props: PivotTableProps) { } const [key, val] = filtersEntries[filtersEntries.length - 1]; + const isMultiSelect = e.metaKey || e.ctrlKey; let updatedFilters = { ...selectedFilters }; - // multi select - // if (selectedFilters && isActiveFilterValue(key, val)) { - // updatedFilters[key] = selectedFilters[key].filter((x: DataRecordValue) => x !== val); - // } else { - // updatedFilters[key] = [...(selectedFilters?.[key] || []), val]; - // } - // single select - if (selectedFilters && isActiveFilterValue(key, val)) { + if (isMultiSelect) { + if (isActiveFilterValue(key, val)) { + updatedFilters[key] = (selectedFilters?.[key] || []).filter( + (x: DataRecordValue) => { + if (x === val) return false; + if (x instanceof Date && val instanceof Date) { + return x.getTime() !== val.getTime(); + } + return true; + }, + ); + } else { + updatedFilters[key] = [...(selectedFilters?.[key] || []), val]; + } + } else if (isActiveFilterValue(key, val)) { updatedFilters = {}; } else { updatedFilters = { @@ -557,7 +573,7 @@ export default function PivotTableChart(props: PivotTableProps) { } handleChange(updatedFilters); }, - [emitCrossFilters, selectedFilters, handleChange], + [emitCrossFilters, isActiveFilterValue, selectedFilters, handleChange], ); const tableOptions = useMemo( diff --git a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx index 9faa71d8455..58f8a529a31 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx @@ -415,7 +415,16 @@ export default function TableChart( const isActiveFilterValue = useCallback( function isActiveFilterValue(key: string, val: DataRecordValue) { - return !!filters && filters[key]?.includes(val); + if (!filters || !filters[key]) return false; + return filters[key].some(filterVal => { + if (filterVal === val) return true; + // DateWithFormatter extends Date — compare by time value + // since memoization cache misses can create new instances + if (filterVal instanceof Date && val instanceof Date) { + return filterVal.getTime() === val.getTime(); + } + return false; + }); }, [filters], ); diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx index 7cc0ef9239a..6988f9afc18 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx @@ -1741,6 +1741,200 @@ describe('plugin-chart-table', () => { ); }); + test('clicking a cell emits cross-filter, clicking again clears it', () => { + const setDataMask = jest.fn(); + const props = transformProps({ + ...testData.basic, + hooks: { setDataMask }, + emitCrossFilters: true, + }); + const { rerender } = render( + + + , + ); + + // Click a string cell to apply cross-filter + const nameCell = screen.getByText('Michael'); + fireEvent.click(nameCell); + + // Find the cross-filter call (not the ownState call) + const crossFilterCall = setDataMask.mock.calls.find( + (call: any[]) => call[0]?.filterState?.filters, + ); + expect(crossFilterCall).toBeDefined(); + const firstCallArg = crossFilterCall![0]; + // Should set the filter + expect(firstCallArg.filterState.filters).toEqual({ + name: ['Michael'], + }); + expect(firstCallArg.extraFormData.filters).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + col: 'name', + op: 'IN', + val: ['Michael'], + }), + ]), + ); + + // Now simulate Redux updating the filters prop (as would happen in dashboard) + setDataMask.mockClear(); + rerender( + + + , + ); + + // The cell should now have the active filter class + const activeCells = document.querySelectorAll('.dt-is-active-filter'); + expect(activeCells.length).toBeGreaterThan(0); + + // Click same cell again to clear cross-filter + setDataMask.mockClear(); + const sameCellAgain = screen.getByText('Michael'); + fireEvent.click(sameCellAgain); + + // Find the cross-filter clearing call + const clearCall = setDataMask.mock.calls.find( + (call: any[]) => call[0]?.filterState !== undefined, + ); + expect(clearCall).toBeDefined(); + const secondCallArg = clearCall![0]; + // Should clear the filter + expect(secondCallArg.filterState.filters).toBeNull(); + expect(secondCallArg.extraFormData.filters).toEqual([]); + }); + + test('cross-filter toggle works with DateWithFormatter values', () => { + const setDataMask = jest.fn(); + const props = transformProps({ + ...testData.basic, + hooks: { setDataMask }, + emitCrossFilters: true, + }); + + // The data has a __timestamp column with DateWithFormatter values + // after processDataRecords. Let's verify this. + const timestampVal = props.data[0].__timestamp; + expect(timestampVal).toBeInstanceOf(DateWithFormatter); + + const { rerender } = render( + + + , + ); + + // Click a timestamp cell - find it by text content + const timestampCell = screen.getByText('2020-01-01 12:34:56'); + fireEvent.click(timestampCell); + + const crossFilterCall = setDataMask.mock.calls.find( + (call: any[]) => call[0]?.filterState?.filters, + ); + expect(crossFilterCall).toBeDefined(); + const firstCallArg = crossFilterCall![0]; + + // Now re-render with the filters from the first click + // This simulates what happens via Redux in the real app + setDataMask.mockClear(); + rerender( + + + , + ); + + // The timestamp cell should be active + const activeCells = document.querySelectorAll('.dt-is-active-filter'); + expect(activeCells.length).toBeGreaterThan(0); + + // Click the same timestamp cell again to clear + setDataMask.mockClear(); + const sameCell = screen.getByText('2020-01-01 12:34:56'); + fireEvent.click(sameCell); + + const clearCall = setDataMask.mock.calls.find( + (call: any[]) => call[0]?.filterState !== undefined, + ); + expect(clearCall).toBeDefined(); + // Should CLEAR the filter (not re-apply it) + expect(clearCall![0].filterState.filters).toBeNull(); + expect(clearCall![0].extraFormData.filters).toEqual([]); + }); + + test('cross-filter toggle clears when DateWithFormatter references differ', () => { + // Regression test: when memoizeOne cache misses between renders, + // new DateWithFormatter instances are created with different references. + // isActiveFilterValue must compare by time value, not reference. + const setDataMask = jest.fn(); + const props = transformProps({ + ...testData.basic, + hooks: { setDataMask }, + emitCrossFilters: true, + }); + + const timestampVal = props.data[0].__timestamp as DateWithFormatter; + expect(timestampVal).toBeInstanceOf(DateWithFormatter); + + // Build filters with a DIFFERENT DateWithFormatter instance (same time value) + const filterKey = '__timestamp'; + const differentRef = new DateWithFormatter(timestampVal.input, { + formatter: timestampVal.formatter, + }); + expect(differentRef).not.toBe(timestampVal); // different reference + expect(differentRef.getTime()).toBe(timestampVal.getTime()); // same time + + const { container } = render( + + + , + ); + + // The cell should show active filter despite different reference + const activeCells = container.querySelectorAll('.dt-is-active-filter'); + expect(activeCells.length).toBeGreaterThan(0); + + // Clicking should CLEAR the filter, not re-apply it + setDataMask.mockClear(); + const timestampCell = screen.getByText('2020-01-01 12:34:56'); + fireEvent.click(timestampCell); + + const clearCall = setDataMask.mock.calls.find( + (call: any[]) => call[0]?.filterState !== undefined, + ); + expect(clearCall).toBeDefined(); + expect(clearCall![0].filterState.filters).toBeNull(); + expect(clearCall![0].extraFormData.filters).toEqual([]); + }); + test('recalculates totals when user filters data', async () => { const formDataWithTotals = { ...testData.basic.formData,