From 947f12ff31af5c8a4ae695cf294f66b8571a2669 Mon Sep 17 00:00:00 2001 From: Raj Zalavadia Date: Thu, 18 Jun 2026 15:47:01 +0530 Subject: [PATCH] [BUGFIX] fix cellSettings not applied when table column filters are active cellConfigs was keyed by row index from the unfiltered `data` array, but the component renders `filteredData` which has different indices after filtering. This caused cellSettings overrides (e.g. "N/A" for null values) to either not appear or apply to the wrong row. Fix: compute cellConfigs from `filteredData` instead of `data` by moving the `columnFilters` state and `filteredData` memo before the `cellConfigs` memo in the hook order. Signed-off-by: Raj Zalavadia --- table/src/components/TablePanel.test.tsx | 117 +++++++++++++++++++++++ table/src/components/TablePanel.tsx | 53 +++++----- table/src/test/mock-query-results.ts | 51 ++++++++++ 3 files changed, 195 insertions(+), 26 deletions(-) diff --git a/table/src/components/TablePanel.test.tsx b/table/src/components/TablePanel.test.tsx index c1925d74..b39f66ad 100644 --- a/table/src/components/TablePanel.test.tsx +++ b/table/src/components/TablePanel.test.tsx @@ -26,6 +26,9 @@ import { VirtuosoMockContext } from 'react-virtuoso'; import { TimeSeriesData } from '@perses-dev/spec'; import { TableOptions, TimeSeriesTableProps } from '../models'; import { + MOCK_MULTI_QUERY_DATA_Q1, + MOCK_MULTI_QUERY_DATA_Q2, + MOCK_MULTI_QUERY_DATA_WITH_ZERO, MOCK_TIME_SERIES_DATA_MULTIVALUE, MOCK_TIME_SERIES_DATA_SINGLEVALUE, MOCK_TIME_SERIES_QUERY_DEFINITION, @@ -351,4 +354,118 @@ describe('TablePanel', () => { TEST_TIMEOUT ); }); + + describe('cellSettings with filtered data', () => { + // Helper that supports multiple query results (simulates multi-query panels like Memory Quota Table) + const renderMultiQueryPanel = (queryData: TimeSeriesData[], options?: TableOptions): void => { + const queryResults = queryData.map((data) => ({ + definition: MOCK_TIME_SERIES_QUERY_DEFINITION, + data, + })); + render( + + + + + + + + + + ); + }; + + const MULTI_QUERY_TABLE_OPTIONS: TableOptions = { + columnSettings: [ + { name: 'timestamp', hide: true }, + { name: 'namespace', header: 'Namespace' }, + { name: 'value #1', header: 'Value 1' }, + { name: 'value #2', header: 'Value 2' }, + ], + cellSettings: [ + { condition: { kind: 'Misc', spec: { value: 'null' } }, text: 'N/A' }, + ], + transforms: [ + { kind: 'MergeSeries', spec: {} }, + { kind: 'JoinByColumnValue', spec: { columns: ['namespace'] } }, + ], + enableFiltering: true, + }; + + it( + 'should show N/A for null values and preserve real values in unfiltered multi-query table', + async () => { + // Q1 has ns-a and ns-b, Q2 has only ns-a → ns-b's value #2 is null + renderMultiQueryPanel([MOCK_MULTI_QUERY_DATA_Q1, MOCK_MULTI_QUERY_DATA_Q2], MULTI_QUERY_TABLE_OPTIONS); + + // ns-a should have both values — verify value #2 = 50 is NOT shown as N/A + expect(await screen.findByRole('cell', { name: 'ns-a' })).toBeInTheDocument(); + expect(await screen.findByRole('cell', { name: '50' })).toBeInTheDocument(); + + // ns-b should show N/A for the missing value #2 + expect(await screen.findByRole('cell', { name: 'ns-b' })).toBeInTheDocument(); + const naCells = await screen.findAllByRole('cell', { name: 'N/A' }); + expect(naCells.length).toBeGreaterThanOrEqual(1); + + // Verify genuine zero (value #1 = 0) would NOT become N/A — 0 is a valid number, not null + // ns-a value #1 = 100, ns-b value #1 = 200 — both are real values, no N/A expected for value #1 + expect(await screen.findByRole('cell', { name: '100' })).toBeInTheDocument(); + expect(await screen.findByRole('cell', { name: '200' })).toBeInTheDocument(); + }, + TEST_TIMEOUT + ); + + it( + 'should show N/A for null values after filtering to a row with missing data', + async () => { + renderMultiQueryPanel([MOCK_MULTI_QUERY_DATA_Q1, MOCK_MULTI_QUERY_DATA_Q2], MULTI_QUERY_TABLE_OPTIONS); + + // Wait for initial render + await screen.findByRole('cell', { name: 'ns-b' }); + + // Apply filter to show only ns-b (which has null for value #2) + const filterButtons = screen.getAllByRole('button', { name: '▼' }); + // First filter button corresponds to the first visible column (namespace) + await userEvent.click(filterButtons[0]!); + + // Select ns-b in the filter dropdown + const nsBCheckbox = await screen.findByRole('checkbox', { name: 'ns-b' }); + await userEvent.click(nsBCheckbox); + + // After filtering to ns-b only, N/A should still appear for the missing value #2 + await waitFor(() => { + const naCells = screen.getAllByRole('cell', { name: 'N/A' }); + expect(naCells.length).toBeGreaterThanOrEqual(1); + }); + + // ns-b's value #1 = 200 should still render correctly (not become N/A) + expect(screen.getByRole('cell', { name: '200' })).toBeInTheDocument(); + }, + TEST_TIMEOUT + ); + + it( + 'should NOT show N/A for genuine zero values', + async () => { + // Q1 has ns-a=100 and ns-b=200, ZERO query has ns-a=50 and ns-b=0 + // ns-b's value #2 is 0 (a real number), not null — must NOT show N/A + renderMultiQueryPanel([MOCK_MULTI_QUERY_DATA_Q1, MOCK_MULTI_QUERY_DATA_WITH_ZERO], MULTI_QUERY_TABLE_OPTIONS); + + // Both namespaces should be present + expect(await screen.findByRole('cell', { name: 'ns-a' })).toBeInTheDocument(); + expect(await screen.findByRole('cell', { name: 'ns-b' })).toBeInTheDocument(); + + // ns-b's value #2 = 0 should render as 0, not N/A + expect(await screen.findByRole('cell', { name: '0' })).toBeInTheDocument(); + + // No N/A should appear — all cells have real values (100, 200, 50, 0) + expect(screen.queryAllByRole('cell', { name: 'N/A' })).toHaveLength(0); + }, + TEST_TIMEOUT + ); + }); }); diff --git a/table/src/components/TablePanel.tsx b/table/src/components/TablePanel.tsx index db5b240a..76e80c41 100644 --- a/table/src/components/TablePanel.tsx +++ b/table/src/components/TablePanel.tsx @@ -565,6 +565,31 @@ export function TablePanel({ contentDimensions, spec, queryResults }: TableProps return columns; }, [keys, spec.columnSettings, spec.defaultColumnHidden, allVariables, gaugeRangeByColumn, spec.cellSettings]); + // Filtering state — declared before cellConfigs so filteredData is available for cell config evaluation + const [columnFilters, setColumnFilters] = useState([]); + + // filter data based on the current filters + const filteredData = useMemo(() => { + let filtered = [...data]; + + // apply column filters if enabled + if (spec.enableFiltering && columnFilters.length > 0) { + filtered = filtered.filter((row) => { + return columnFilters.every((filter) => { + const value = row[filter.id]; + const filterValues = filter.value as Array; + + if (!filterValues || filterValues.length === 0) return true; // No filter values means no filtering + + // Check if the row value is in the selected filter values + return filterValues.includes(value as string | number); + }); + }); + } + + return filtered; + }, [data, columnFilters, spec.enableFiltering]); + // Generate cell settings that will be used by the table to render cells (text color, background color, ...) const cellConfigs: TableCellConfigs = useMemo(() => { // If there are no cell settings globally or per column, return an empty object @@ -575,7 +600,7 @@ export function TablePanel({ contentDimensions, spec, queryResults }: TableProps const result: TableCellConfigs = {}; let index = 0; - for (const row of data) { + for (const row of filteredData) { // Transforming key to object to extend the row with undefined values if the key is not present // for checking the cell config "Misc" condition with "null" const keysAsObj = keys.reduce( @@ -614,7 +639,7 @@ export function TablePanel({ contentDimensions, spec, queryResults }: TableProps } return result; - }, [data, keys, spec.cellSettings, spec.columnSettings]); + }, [filteredData, keys, spec.cellSettings, spec.columnSettings]); function generateDefaultSortingState(): SortingState { return ( @@ -631,8 +656,6 @@ export function TablePanel({ contentDimensions, spec, queryResults }: TableProps const [sorting, setSorting] = useState(generateDefaultSortingState()); - // Filtering state - const [columnFilters, setColumnFilters] = useState([]); const [filterAnchorEl, setFilterAnchorEl] = useState<{ [key: string]: HTMLElement | null }>({}); const [openFilterColumn, setOpenFilterColumn] = useState(null); @@ -685,28 +708,6 @@ export function TablePanel({ contentDimensions, spec, queryResults }: TableProps }; }, [openFilterColumn]); - // filter data based on the current filters - const filteredData = useMemo(() => { - let filtered = [...data]; - - // apply column filters if enabled - if (spec.enableFiltering && columnFilters.length > 0) { - filtered = filtered.filter((row) => { - return columnFilters.every((filter) => { - const value = row[filter.id]; - const filterValues = filter.value as Array; - - if (!filterValues || filterValues.length === 0) return true; // No filter values means no filtering - - // Check if the row value is in the selected filter values - return filterValues.includes(value as string | number); - }); - }); - } - - return filtered; - }, [data, columnFilters, spec.enableFiltering]); - // Keep ref in sync with filtered data for use in selection handler filteredDataRef.current = filteredData; diff --git a/table/src/test/mock-query-results.ts b/table/src/test/mock-query-results.ts index 479031aa..f07f88e0 100644 --- a/table/src/test/mock-query-results.ts +++ b/table/src/test/mock-query-results.ts @@ -275,6 +275,57 @@ export const MOCK_NULL_QUERY_RESULT = [ }, ]; +// Two queries where Query 1 covers both namespaces but Query 2 only covers one. +// After MergeSeries + JoinByColumnValue, ns-b will have null for value #2. +export const MOCK_MULTI_QUERY_DATA_Q1: TimeSeriesData = { + timeRange: { start: new Date(1666625535000), end: new Date(1666625535000) }, + stepMs: 24379, + series: [ + { + name: 'namespace="ns-a"', + values: [[1666479357903, 100]], + labels: { namespace: 'ns-a' }, + }, + { + name: 'namespace="ns-b"', + values: [[1666479357903, 200]], + labels: { namespace: 'ns-b' }, + }, + ], +}; + +export const MOCK_MULTI_QUERY_DATA_Q2: TimeSeriesData = { + timeRange: { start: new Date(1666625535000), end: new Date(1666625535000) }, + stepMs: 24379, + series: [ + { + name: 'namespace="ns-a"', + values: [[1666479357903, 50]], + labels: { namespace: 'ns-a' }, + }, + // ns-b intentionally absent — simulates missing data + ], +}; + +// Query where a namespace has genuine zero value (not null). +// Used to verify 0 is rendered as 0, not as N/A. +export const MOCK_MULTI_QUERY_DATA_WITH_ZERO: TimeSeriesData = { + timeRange: { start: new Date(1666625535000), end: new Date(1666625535000) }, + stepMs: 24379, + series: [ + { + name: 'namespace="ns-a"', + values: [[1666479357903, 50]], + labels: { namespace: 'ns-a' }, + }, + { + name: 'namespace="ns-b"', + values: [[1666479357903, 0]], + labels: { namespace: 'ns-b' }, + }, + ], +}; + export const MOCK_TIME_SERIES_QUERY_DEFINITION = { kind: 'TimeSeriesQuery', spec: {