diff --git a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx index de20a89e34b9..84c920456c4f 100644 --- a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx +++ b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx @@ -1985,45 +1985,174 @@ test('pending LIKE debounce still applies after rerender recreates updateDataMas reduxState, ); - fireEvent.change( - screen.getByPlaceholderText('Type to search (contains)...'), - { - target: { value: 'Jen' }, + const input = screen.getByPlaceholderText('Type to search (contains)...'); + + // Batch the keystroke and the rerender into one update so the deferred + // value hasn't settled yet when updateDataMask's identity changes -- + // useDeferredValue has no fixed delay, so letting the keystroke flush on + // its own (as a real fixed-delay debounce would still be pending) would + // settle it before the rerender and not exercise the stale-ref path. + act(() => { + fireEvent.change(input, { target: { value: 'Jen' } }); + rerender( + , + ); + }); + + await waitFor(() => { + expect(setDataMaskMock).toHaveBeenCalledWith( + expect.objectContaining({ + extraFormData: { + filters: [ + { + col: 'gender', + op: 'ILIKE', + val: '%Jen%', + }, + ], + }, + }), + ); + }); +}); + +test('pending LIKE debounce is canceled when operatorType switches back to Exact', async () => { + jest.useFakeTimers({ advanceTimers: true }); + const setDataMaskMock = jest.fn(); + const likeProps = buildSelectFilterProps({ + formData: { operatorType: SelectFilterOperatorType.Contains }, + filterState: { value: undefined }, + setDataMask: setDataMaskMock, + }); + + const reduxState = { + useRedux: true, + initialState: { + nativeFilters: { + filters: { 'test-filter': { name: 'Test Filter' } }, + }, + dataMask: { + 'test-filter': { + extraFormData: {}, + filterState: { value: undefined }, + }, + }, }, + }; + + const { rerender } = render( + , + reduxState, ); - setDataMaskMock.mockClear(); + const input = screen.getByPlaceholderText('Type to search (contains)...'); + const callsBeforeSwitch = setDataMaskMock.mock.calls.length; - rerender( - , - ); + // Same batching as the sibling test above: the keystroke and the switch + // away from the LIKE operator land in the same update, before the + // deferred value has a chance to settle on its own. + act(() => { + fireEvent.change(input, { target: { value: 'Jen' } }); + rerender( + , + ); + }); + await act(async () => {}); act(() => { - jest.advanceTimersByTime(500); + jest.advanceTimersByTime(1000); }); - expect(setDataMaskMock).toHaveBeenCalledWith( + // Rerendering with fresh (non-memoized) props can independently re-run + // this plugin's own default-value initialisation, unrelated to the LIKE + // debounce this test is pinning -- so assert no *new* call carries the + // 'Jen' ILIKE payload, rather than that setDataMask was never called again. + const callsSinceSwitch = setDataMaskMock.mock.calls + .slice(callsBeforeSwitch) + .map(call => call[0]); + expect(callsSinceSwitch).not.toContainEqual( expect.objectContaining({ - extraFormData: { - filters: [ - { - col: 'gender', - op: 'ILIKE', - val: '%Jen%', - }, - ], + extraFormData: expect.objectContaining({ + filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })], + }), + }), + ); +}); + +test('switching Exact then back to LIKE does not replay the discarded pending edit', async () => { + jest.useFakeTimers({ advanceTimers: true }); + const setDataMaskMock = jest.fn(); + const buildProps = (operatorType: SelectFilterOperatorType) => + buildSelectFilterProps({ + formData: { operatorType }, + filterState: { value: undefined }, + setDataMask: setDataMaskMock, + }); + const reduxState = { + useRedux: true, + initialState: { + nativeFilters: { + filters: { 'test-filter': { name: 'Test Filter' } }, }, + dataMask: { + 'test-filter': { + extraFormData: {}, + filterState: { value: undefined }, + }, + }, + }, + }; + + const { rerender } = render( + , + reduxState, + ); + const input = screen.getByPlaceholderText('Type to search (contains)...'); + + act(() => { + fireEvent.change(input, { target: { value: 'Jen' } }); + rerender( + , + ); + }); + await act(async () => {}); + act(() => { + jest.advanceTimersByTime(1000); + }); + + const callsBeforeReturn = setDataMaskMock.mock.calls.length; + rerender( + , + ); + await act(async () => {}); + act(() => { + jest.advanceTimersByTime(1000); + }); + + expect( + setDataMaskMock.mock.calls.slice(callsBeforeReturn).map(call => call[0]), + ).not.toContainEqual( + expect.objectContaining({ + extraFormData: expect.objectContaining({ + filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })], + }), }), ); }); -test('pending LIKE debounce is canceled when operatorType switches back to Exact', async () => { +test('clear-all does not restore a LIKE edit still pending in the deferred value', async () => { jest.useFakeTimers({ advanceTimers: true }); const setDataMaskMock = jest.fn(); const likeProps = buildSelectFilterProps({ @@ -2031,7 +2160,6 @@ test('pending LIKE debounce is canceled when operatorType switches back to Exact filterState: { value: undefined }, setDataMask: setDataMaskMock, }); - const reduxState = { useRedux: true, initialState: { @@ -2051,31 +2179,169 @@ test('pending LIKE debounce is canceled when operatorType switches back to Exact , reduxState, ); + const input = screen.getByPlaceholderText('Type to search (contains)...'); - fireEvent.change( - screen.getByPlaceholderText('Type to search (contains)...'), - { - target: { value: 'Jen' }, + // Type and fire clear-all in one update, then release the trigger, so the + // deferred value still holds the typed text when the trigger goes away. + act(() => { + fireEvent.change(input, { target: { value: 'Jen' } }); + rerender( + , + ); + }); + const callsBeforeRelease = setDataMaskMock.mock.calls.length; + rerender(); + await act(async () => {}); + act(() => { + jest.advanceTimersByTime(1000); + }); + + expect(input).toHaveValue(''); + expect( + setDataMaskMock.mock.calls.slice(callsBeforeRelease).map(call => call[0]), + ).not.toContainEqual( + expect.objectContaining({ + extraFormData: expect.objectContaining({ + filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })], + }), + }), + ); +}); + +const likeReduxState = { + useRedux: true, + initialState: { + nativeFilters: { + filters: { 'test-filter': { name: 'Test Filter' } }, + }, + dataMask: { + 'test-filter': { + extraFormData: {}, + filterState: { value: undefined }, + }, }, + }, +}; + +test('switching match type discards the pending LIKE edit and clears the displayed text', async () => { + jest.useFakeTimers({ advanceTimers: true }); + const setDataMaskMock = jest.fn(); + const buildProps = (operatorType: SelectFilterOperatorType) => + buildSelectFilterProps({ + formData: { operatorType }, + filterState: { value: undefined }, + setDataMask: setDataMaskMock, + }); + + const { rerender } = render( + , + likeReduxState, ); + const input = screen.getByPlaceholderText('Type to search (contains)...'); + + act(() => { + fireEvent.change(input, { target: { value: 'Jen' } }); + rerender( + , + ); + }); + await act(async () => {}); + act(() => { + jest.advanceTimersByTime(1000); + }); + + expect( + screen.getByPlaceholderText('Type to search (starts with)...'), + ).toHaveValue(''); +}); + +test('re-entering the same LIKE text after a parent reset commits a new mask', async () => { + jest.useFakeTimers({ advanceTimers: true }); + const setDataMaskMock = jest.fn(); + const buildProps = (value: string[] | undefined, defaultValue?: string[]) => + buildSelectFilterProps({ + formData: { + operatorType: SelectFilterOperatorType.Contains, + defaultValue, + }, + filterState: { value }, + setDataMask: setDataMaskMock, + }); + const containsJen = expect.objectContaining({ + extraFormData: expect.objectContaining({ + filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })], + }), + }); + + const { rerender } = render( + , + likeReduxState, + ); + const input = screen.getByPlaceholderText('Type to search (contains)...'); + + fireEvent.change(input, { target: { value: 'Jen' } }); + act(() => { + jest.advanceTimersByTime(1000); + }); + expect(setDataMaskMock).toHaveBeenCalledWith(containsJen); + + // The parent echoes the committed value, then resets it to a different + // default, which moves the committed mask away from 'Jen'. + rerender(); + rerender(); + await act(async () => {}); + expect(input).toHaveValue(''); setDataMaskMock.mockClear(); + fireEvent.change(input, { target: { value: 'Jen' } }); + await act(async () => {}); + act(() => { + jest.advanceTimersByTime(1000); + }); + await act(async () => {}); - rerender( - , + expect(setDataMaskMock).toHaveBeenCalledWith(containsJen); +}); + +test('Clear All publishes an empty server search after a settled search', async () => { + jest.useFakeTimers({ advanceTimers: true }); + const setDataMaskMock = jest.fn(); + const props = buildSelectFilterProps({ + formData: { searchAllOptions: true, multiSelect: true }, + filterState: { value: ['boy'] }, + setDataMask: setDataMaskMock, + }); + + const { rerender } = render( + , + likeReduxState, ); + const combobox = screen.getAllByRole('combobox')[0]; + await userEvent.click(combobox); + await userEvent.type(combobox, 'ab'); + act(() => { + jest.advanceTimersByTime(1000); + }); + await waitFor(() => { + const lastCall = setDataMaskMock.mock.calls.at(-1)?.[0]; + expect(lastCall?.ownState?.search).toBe('ab'); + }); + rerender( + , + ); + await act(async () => {}); act(() => { - jest.advanceTimersByTime(500); + jest.advanceTimersByTime(1000); }); - expect(setDataMaskMock).not.toHaveBeenCalled(); + const lastCall = setDataMaskMock.mock.calls.at(-1)?.[0]; + expect(lastCall?.ownState?.search).toBe(''); }); test('renders standard Select dropdown when operatorType is Exact', () => { diff --git a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx index 551e7a02ab68..dcdb72bfc043 100644 --- a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx +++ b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx @@ -17,7 +17,14 @@ * under the License. */ /* eslint-disable no-param-reassign */ -import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { + useCallback, + useDeferredValue, + useEffect, + useMemo, + useRef, + useState, +} from 'react'; import { t, tn } from '@apache-superset/core/translation'; import { AppSection, @@ -174,7 +181,21 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { ); const [col] = groupby; const [initialColtypeMap] = useState(coltypeMap); - const [search, setSearch] = useState(''); + const [searchInput, setSearchInput] = useState(''); + const deferredSearchInput = useDeferredValue(searchInput); + // Trails searchInput by a quiet period; only this value drives the server + // search dispatch, since useDeferredValue lowers render priority but does + // not coalesce keystrokes by time. + const [settledSearch, setSettledSearch] = useState(''); + const settleSearch = useMemo( + () => debounce(setSettledSearch, Constants.SLOW_DEBOUNCE), + [], + ); + useEffect(() => () => settleSearch.cancel(), [settleSearch]); + // Set true the first time the user actually searches; distinguishes a real + // search from the clearAllTrigger reset below, which must not re-trigger + // the searchAllOptions dispatch once the deferred value catches up. + const hasSearchedRef = useRef(false); const userClearedRef = useRef(false); const [dataMask, dispatchDataMask] = useImmerReducer(reducer, { extraFormData: {}, @@ -200,11 +221,23 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { const [likeInputValue, setLikeInputValue] = useState( filterState.value?.[0] != null ? String(filterState.value[0]) : '', ); + // null until the user types in the LIKE input; distinguishes a real edit + // from the clearAllTrigger reset below, which also clears likeInputValue + // but must not re-trigger a dispatch once the deferred value catches up. + const [pendingLikeValue, setPendingLikeValue] = useState(null); + const settleLikeValue = useMemo( + () => debounce(setPendingLikeValue, Constants.SLOW_DEBOUNCE), + [], + ); + useEffect(() => () => settleLikeValue.cancel(), [settleLikeValue]); useEffect(() => { const externalValue = filterState.value?.[0] != null ? String(filterState.value[0]) : ''; setLikeInputValue(externalValue); + // A parent-driven value change supersedes any settled edit, so re-entering + // the same text afterwards registers as a fresh edit. + setPendingLikeValue(null); }, [filterState.value]); const prevExcludeFilterValues = useRef(excludeFilterValues); @@ -280,32 +313,62 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { const isDisabled = appSection === AppSection.FilterConfigModal && defaultToFirstItem; - const onSearch = useMemo( - () => - debounce((search: string) => { - setSearch(search); - if (searchAllOptions) { - dispatchDataMask({ - type: 'ownState', - ownState: { - coltypeMap: initialColtypeMap, - // The dropdown offers `stripSurroundingQuotes(search)` as the - // creatable option, so the server has to be asked for the same - // string or the two disagree about what was searched for. - search: stripSurroundingQuotes(search).trim(), - }, - }); - } - }, Constants.SLOW_DEBOUNCE), - [dispatchDataMask, initialColtypeMap, searchAllOptions], + const handleSearch = useCallback( + (value: string) => { + hasSearchedRef.current = true; + setSearchInput(value); + settleSearch(value); + }, + [settleSearch], ); + // Read through a ref so a parent re-render that hands down new identities + // for these doesn't, by itself, re-fire the dispatch below -- only an + // actual search settling (a change to deferredSearchInput) should. + const searchDispatchRef = useRef({ + searchAllOptions, + dispatchDataMask, + initialColtypeMap, + }); + useEffect(() => { + searchDispatchRef.current = { + searchAllOptions, + dispatchDataMask, + initialColtypeMap, + }; + }); + + useEffect(() => { + if (!hasSearchedRef.current) { + return; + } + const { + searchAllOptions: currentSearchAllOptions, + dispatchDataMask: currentDispatchDataMask, + initialColtypeMap: currentInitialColtypeMap, + } = searchDispatchRef.current; + if (!currentSearchAllOptions) { + return; + } + currentDispatchDataMask({ + type: 'ownState', + ownState: { + coltypeMap: currentInitialColtypeMap, + // The dropdown offers `stripSurroundingQuotes(search)` as the + // creatable option, so the server has to be asked for the same + // string or the two disagree about what was searched for. + search: stripSurroundingQuotes(settledSearch).trim(), + }, + }); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [settledSearch]); + const handleBlur = useCallback(() => { unsetFocusedFilter(); - if (search) { - onSearch(''); + if (searchInput) { + handleSearch(''); } - }, [onSearch, search, unsetFocusedFilter]); + }, [handleSearch, searchInput, unsetFocusedFilter]); const handleChange = useCallback( (value?: SelectValue | number | string) => { @@ -375,7 +438,7 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { }, [data, datatype, col, labelFormatter, creatable, filterState.value]); const options = useMemo(() => { - const unquotedSearch = stripSurroundingQuotes(search); + const unquotedSearch = stripSurroundingQuotes(deferredSearchInput); if ( unquotedSearch && creatable !== false && @@ -387,7 +450,7 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { ]; } return uniqueOptions; - }, [search, uniqueOptions, creatable]); + }, [deferredSearchInput, uniqueOptions, creatable]); const sortComparator = useCallback( (a: LabeledValue, b: LabeledValue) => { @@ -525,8 +588,26 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { }); updateDataMask(null); - setSearch(''); + settleSearch.cancel(); + settleLikeValue.cancel(); + // A search the user already ran left a server-side subset in ownState; + // publish the empty search so the cleared filter lists every option. + const { + searchAllOptions: currentSearchAllOptions, + dispatchDataMask: currentDispatchDataMask, + initialColtypeMap: currentInitialColtypeMap, + } = searchDispatchRef.current; + if (hasSearchedRef.current && currentSearchAllOptions) { + currentDispatchDataMask({ + type: 'ownState', + ownState: { coltypeMap: currentInitialColtypeMap, search: '' }, + }); + } + setSearchInput(''); + setSettledSearch(''); + hasSearchedRef.current = false; setLikeInputValue(''); + setPendingLikeValue(null); onClearAllComplete?.(formData.nativeFilterId); } }, [clearAllTrigger, onClearAllComplete, updateDataMask]); @@ -564,32 +645,37 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { updateDataMaskRef.current = updateDataMask; }, [updateDataMask]); - const debouncedLikeChange = useMemo( - () => - debounce((text: string) => { - if (text) { - updateDataMaskRef.current([text]); - } else { - updateDataMaskRef.current(null); - } - }, Constants.SLOW_DEBOUNCE), - [], - ); + // Any match-type change (including between two LIKE modes) discards the + // pending edit, so text typed under the previous mode is never replayed and + // re-entering the same text afterwards registers as a fresh edit. + // The displayed text reverts to the committed value too, so a discarded edit + // never lingers in the input without a matching filter value. + const committedLikeValueRef = useRef(''); + committedLikeValueRef.current = + filterState.value?.[0] != null ? String(filterState.value[0]) : ''; + useEffect(() => { + settleLikeValue.cancel(); + setPendingLikeValue(null); + setLikeInputValue(committedLikeValueRef.current); + }, [operatorType, isLikeOperator, settleLikeValue]); useEffect(() => { - if (!isLikeOperator || clearAllTrigger) { - debouncedLikeChange.cancel(); + if (pendingLikeValue === null || !isLikeOperator || clearAllTrigger) { + return; } - }, [clearAllTrigger, debouncedLikeChange, isLikeOperator]); - - useEffect(() => () => debouncedLikeChange.cancel(), [debouncedLikeChange]); + if (pendingLikeValue) { + updateDataMaskRef.current([pendingLikeValue]); + } else { + updateDataMaskRef.current(null); + } + }, [pendingLikeValue, isLikeOperator, clearAllTrigger]); const handleLikeInputChange = useCallback( (e: React.ChangeEvent) => { setLikeInputValue(e.target.value); - debouncedLikeChange(e.target.value); + settleLikeValue(e.target.value); }, - [debouncedLikeChange], + [settleLikeValue], ); const getSelectPopupContainer = useCallback( @@ -669,8 +755,8 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { mode={multiSelect ? 'multiple' : 'single'} placeholder={placeholderText} helperText={helperText} - onClear={() => onSearch('')} - onSearch={onSearch} + onClear={() => handleSearch('')} + onSearch={handleSearch} onBlur={handleBlur} onFocus={setFocusedFilter} onMouseEnter={setHoveredFilter}