From 7e2db430d2d7c4ecbd8bc8435c89f1874100f528 Mon Sep 17 00:00:00 2001 From: Kunal Sharma Date: Fri, 4 Sep 2026 07:13:37 +0000 Subject: [PATCH] fix(native-filters): stabilize DefaultValue's filterState object identity DefaultValue.tsx rebuilt its `filterState` prop as a brand new object literal on every render: filterState={{ ...formFilter?.defaultDataMask?.filterState, validateMessage: ..., validateStatus: ..., }} That object flows down through SuperChart into the native filter plugin and ultimately becomes the `value` prop of the underlying Select component. Giving it a new identity on every render it any parent re-render triggers (the config modal calls forceUpdate() after every dataMask change, including pure search/ownState updates while the user is typing) is exactly the pattern flagged as the root cause in #43347's investigation: an unstable filterState reference feeding into a component that treats identity changes as "the value changed". Memoize the object on its underlying source plus the two derived validation fields so it keeps the same reference across renders that don't actually change anything, and only gets a new one when the value, label, or validation state genuinely changes. Fixes #43717 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015kAvbZ6SeKgp6jbGcvpXSh --- .../FiltersConfigForm/DefaultValue.test.tsx | 118 ++++++++++++++++++ .../FiltersConfigForm/DefaultValue.tsx | 25 +++- 2 files changed, 138 insertions(+), 5 deletions(-) create mode 100644 superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.test.tsx diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.test.tsx new file mode 100644 index 000000000000..f598256be74d --- /dev/null +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.test.tsx @@ -0,0 +1,118 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { render } from 'spec/helpers/testing-library'; +import DefaultValue from './DefaultValue'; + +const capturedFilterStates: unknown[] = []; + +jest.mock('@superset-ui/core', () => { + const original = jest.requireActual('@superset-ui/core'); + return { + ...original, + SuperChart: (props: Record) => { + capturedFilterStates.push(props.filterState); + return
; + }, + }; +}); + +const FILTER_ID = 'filter-1'; + +// A minimal stand-in for antd's FormInstance: DefaultValue only ever calls +// `form.getFieldValue('filters')`. +const makeForm = (filtersValue: Record) => ({ + getFieldValue: (name: string) => + name === 'filters' ? filtersValue : undefined, +}); + +const baseProps = { + hasDefaultValue: true, + filterId: FILTER_ID, + setDataMask: jest.fn(), + hasDataset: true, + formData: { filterType: 'filter_select' } as any, + enableNoResults: true, +}; + +beforeEach(() => { + capturedFilterStates.length = 0; +}); + +test('keeps the same filterState object reference across renders that do not change its contents', () => { + // Same underlying filterState object on every call to getFieldValue, + // exactly like re-opening the form without touching the field. + const filterState = { value: [1, 2], label: 'One, Two' }; + const formFilter = { + filterType: 'filter_select', + defaultValueQueriesData: [{ data: [{ col: 1 }, { col: 2 }] }], + defaultDataMask: { filterState }, + }; + + const { rerender } = render( + , + ); + + // A parent re-render triggered by something unrelated (e.g. the config + // modal's forceUpdate() after an ownState-only dataMask change while the + // user is typing/searching) with the exact same underlying form data. + rerender( + , + ); + + expect(capturedFilterStates).toHaveLength(2); + // Before the fix, DefaultValue spread `filterState` into a brand new + // object literal on every render, so this would be two distinct objects + // (even though their contents matched) — and the underlying Select + // resets its selection whenever the object it receives changes identity. + expect(capturedFilterStates[0]).toBe(capturedFilterStates[1]); +}); + +test('produces a new filterState object once the underlying value actually changes', () => { + const formFilterWithValue = (value: number[]) => ({ + filterType: 'filter_select', + defaultValueQueriesData: [{ data: [{ col: 1 }, { col: 2 }] }], + defaultDataMask: { filterState: { value, label: value.join(', ') } }, + }); + + const { rerender } = render( + , + ); + + rerender( + , + ); + + expect(capturedFilterStates).toHaveLength(2); + expect(capturedFilterStates[0]).not.toBe(capturedFilterStates[1]); + expect( + (capturedFilterStates[1] as { value: number[] }).value, + ).toEqual([1, 2, 3]); +}); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx index 3d3d5e4d77f0..996179533fd2 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx @@ -62,6 +62,25 @@ const DefaultValue: FC = ({ const value = formFilter?.defaultDataMask?.filterState?.value; const isMissingRequiredValue = hasDefaultValue && (value === null || value === undefined); + const baseFilterState = formFilter?.defaultDataMask?.filterState; + + // Every DefaultValue render used to spread `baseFilterState` into a brand + // new object literal here, so the `filterState` prop the underlying Select + // receives got a new identity on every render (e.g. while the user is + // typing/searching), even when its actual contents were unchanged. Select + // resets its internal selection whenever that identity changes, which is + // what wiped out already-chosen default values. Memoizing on the + // underlying reference plus the two derived validation fields keeps the + // object stable across renders that don't actually change anything. + const filterState = useMemo( + () => ({ + ...baseFilterState, + validateMessage: isMissingRequiredValue && t('Value is required'), + validateStatus: isMissingRequiredValue && 'error', + }), + // eslint-disable-next-line react-hooks/exhaustive-deps + [baseFilterState, isMissingRequiredValue], + ); return loading ? ( @@ -76,11 +95,7 @@ const DefaultValue: FC = ({ chartType={chartType} hooks={{ setDataMask }} enableNoResults={enableNoResults} - filterState={{ - ...formFilter?.defaultDataMask?.filterState, - validateMessage: isMissingRequiredValue && t('Value is required'), - validateStatus: isMissingRequiredValue && 'error', - }} + filterState={filterState} /> ); };