fix(table): fix cross-filter not clearing on second click in Interactive Table (#39253)

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2026-04-17 07:05:04 +00:00 · 2026-04-15 10:30:36 -07:00
parent 44e77fdf2b
commit c2d96e0dce
8 changed files with 369 additions and 54 deletions
--- 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,
--- 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<string, DataRecordValue[]> | null;
  renderTimeComparisonDropdown: () => JSX.Element | null;
  cleanedTotals: DataRecord;
  showTotals: boolean;
@@ -135,8 +136,9 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo(
    percentMetrics,
    serverPageLength,
    hasServerPageLengthChanged,
-    handleCrossFilter,
-    isActiveFilterValue,
+    handleCellClicked,
+    handleSelectionChanged,
+    filters,
    renderTimeComparisonDropdown,
    cleanedTotals,
    showTotals,
@@ -422,6 +424,15 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = 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<AgGridTableProps> = 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<AgGridTableProps> = memo(
            initialSortState: getInitialSortState(
              serverPaginationData?.sortBy || [],
            ),
-            isActiveFilterValue,
            lastFilteredColumn: serverPaginationData?.lastFilteredColumn,
            lastFilteredInputPosition:
              serverPaginationData?.lastFilteredInputPosition,
--- 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<D extends DataRecord = DataRecord>(

  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<D extends DataRecord = DataRecord>(
    [timeGrain, isRawRecords],
  );

-  const toggleFilter = useCallback(
-    (event: CellClickedEvent | IMenuActionParams) => {
+  const activeColumnRef = useRef<string | null>(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<D extends DataRecord = DataRecord>(
        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
        }
--- 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';
    }
--- 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,
--- 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(
--- 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<D extends DataRecord = DataRecord>(

  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],
  );
--- 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(
+          <ProviderWrapper>
+            <TableChart
+              {...props}
+              emitCrossFilters
+              setDataMask={setDataMask}
+              sticky={false}
+            />
+          </ProviderWrapper>,
+        );
+
+        // 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(
+          <ProviderWrapper>
+            <TableChart
+              {...props}
+              emitCrossFilters
+              setDataMask={setDataMask}
+              filters={{ name: ['Michael'] }}
+              sticky={false}
+            />
+          </ProviderWrapper>,
+        );
+
+        // 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(
+          <ProviderWrapper>
+            <TableChart
+              {...props}
+              emitCrossFilters
+              setDataMask={setDataMask}
+              sticky={false}
+            />
+          </ProviderWrapper>,
+        );
+
+        // 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(
+          <ProviderWrapper>
+            <TableChart
+              {...props}
+              emitCrossFilters
+              setDataMask={setDataMask}
+              filters={firstCallArg.filterState.filters}
+              sticky={false}
+            />
+          </ProviderWrapper>,
+        );
+
+        // 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(
+          <ProviderWrapper>
+            <TableChart
+              {...props}
+              emitCrossFilters
+              setDataMask={setDataMask}
+              filters={{ [filterKey]: [differentRef] }}
+              sticky={false}
+            />
+          </ProviderWrapper>,
+        );
+
+        // 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,