diff --git a/superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx b/superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx index 48264a054280..c2ecee904db3 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx +++ b/superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx @@ -897,6 +897,476 @@ test('fires onChange when pasting a selection', async () => { await waitFor(() => expect(onChange).toHaveBeenCalledTimes(1)); }); +test('replaces cached options with search results instead of merging', async () => { + const page0Data = Array.from({ length: 10 }, (_, i) => ({ + label: `Option ${i}`, + value: i, + })); + const searchData = [{ label: 'Search Match', value: 100 }]; + const loadOptions = jest.fn(async (search: string) => { + if (search === '') { + return { data: page0Data, totalCount: 100 }; + } + return { data: searchData, totalCount: 1 }; + }); + + render(); + await open(); + await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(1)); + + let options = await findAllSelectOptions(); + expect(options).toHaveLength(10); + + await type('search'); + await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(2)); + + options = await findAllSelectOptions(); + expect(options).toHaveLength(1); + expect(options[0]).toHaveTextContent('Search Match'); +}); + +test('shows all options when filterOption is false', async () => { + const page0Data = Array.from({ length: 10 }, (_, i) => ({ + label: `Base ${i}`, + value: i, + })); + const searchData = Array.from({ length: 5 }, (_, i) => ({ + label: `Server ${i}`, + value: 100 + i, + })); + const loadOptions = jest.fn(async (search: string) => + search === '' + ? { data: page0Data, totalCount: 100 } + : { data: searchData, totalCount: 5 }, + ); + + render( + , + ); + await open(); + await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(1)); + + await type('zzz_no_match'); + await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(2)); + + const options = await findAllSelectOptions(); + expect(options).toHaveLength(5); + expect(options[0]).toHaveTextContent('Server 0'); +}); + +test('preserves new option entry across search fetch when allowNewOptions is on', async () => { + const page0Data = Array.from({ length: 10 }, (_, i) => ({ + label: `Option ${i}`, + value: i, + })); + const loadOptions = jest.fn(async (search: string) => { + if (search === '') { + return { data: page0Data, totalCount: 100 }; + } + return { data: [], totalCount: 0 }; + }); + + render( + , + ); + await open(); + await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(1)); + + await type('newval'); + await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(2)); + + const options = await findAllSelectOptions(); + expect(options).toHaveLength(1); + expect(options[0]).toHaveTextContent('newval'); + // Stale page-0 options must not bleed through. + expect(screen.queryByText('Option 0')).not.toBeInTheDocument(); +}); + +test('restores base options when search is cleared', async () => { + const page0Data = Array.from({ length: 10 }, (_, i) => ({ + label: `Option ${i}`, + value: i, + })); + const searchData = [{ label: 'Search Match', value: 100 }]; + const loadOptions = jest.fn(async (search: string) => { + if (search === '') { + return { data: page0Data, totalCount: 100 }; + } + return { data: searchData, totalCount: 1 }; + }); + + render(); + await open(); + await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(1)); + + await type('search'); + await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(2)); + let options = await findAllSelectOptions(); + expect(options).toHaveLength(1); + expect(options[0]).toHaveTextContent('Search Match'); + + // type() clears the input before typing, so passing '' clears the search. + await type(''); + await waitFor(async () => { + options = await findAllSelectOptions(); + expect(options).toHaveLength(10); + }); + expect(options[0]).toHaveTextContent('Option 0'); + expect(screen.queryByText('Search Match')).not.toBeInTheDocument(); +}); + +test('replaces results when switching between two searches', async () => { + const page0Data = Array.from({ length: 10 }, (_, i) => ({ + label: `Option ${i}`, + value: i, + })); + const loadOptions = jest.fn(async (search: string) => { + if (search === '') { + return { data: page0Data, totalCount: 100 }; + } + return { + data: [{ label: `Match-${search}`, value: `v-${search}` }], + totalCount: 1, + }; + }); + + render(); + await open(); + await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(1)); + + await type('alpha'); + await waitFor(async () => { + const options = await findAllSelectOptions(); + expect(options).toHaveLength(1); + expect(options[0]).toHaveTextContent('Match-alpha'); + }); + + await type('beta'); + await waitFor(async () => { + const options = await findAllSelectOptions(); + expect(options).toHaveLength(1); + expect(options[0]).toHaveTextContent('Match-beta'); + }); + expect(screen.queryByText('Match-alpha')).not.toBeInTheDocument(); +}); + +test('refetches a dropped search response when the same search is repeated', async () => { + type OptionRow = { label: string; value: string | number }; + type PageResponse = { data: OptionRow[]; totalCount: number }; + // Resolves the in-flight loadOptions promise of the calling test. + let resolveAlpha: ((value: PageResponse) => void) | null = null; + const page0Data: OptionRow[] = Array.from({ length: 10 }, (_, i) => ({ + label: `Option ${i}`, + value: i, + })); + const alphaData: OptionRow[] = [{ label: 'Match-alpha', value: 'va' }]; + const betaData: OptionRow[] = [{ label: 'Match-beta', value: 'vb' }]; + + const loadOptions = jest.fn((search: string) => { + if (search === '') { + return Promise.resolve({ + data: page0Data, + totalCount: 100, + }); + } + if (search === 'alpha') { + // First call: hold the promise so it resolves only after beta returns. + // Second call (after beta): resolve immediately so the cache MUST allow + // a refetch. + if (!resolveAlpha) { + return new Promise(resolve => { + resolveAlpha = resolve; + }); + } + return Promise.resolve({ data: alphaData, totalCount: 1 }); + } + return Promise.resolve({ data: betaData, totalCount: 1 }); + }); + + render(); + await open(); + await waitFor(() => expect(loadOptions).toHaveBeenCalledWith('', 0, 10)); + + await type('alpha'); + await waitFor(() => expect(loadOptions).toHaveBeenCalledWith('alpha', 0, 10)); + // alpha's promise is held; switch to beta which resolves first. + await type('beta'); + await waitFor(async () => { + const options = await findAllSelectOptions(); + expect(options).toHaveLength(1); + expect(options[0]).toHaveTextContent('Match-beta'); + }); + + // Release the stale alpha response. It must be dropped — its key must not + // be cached, or returning to "alpha" later would short-circuit the fetch. + resolveAlpha!({ data: alphaData, totalCount: 1 }); + await waitFor(async () => { + // Beta is still showing because alpha's response was dropped. + const options = await findAllSelectOptions(); + expect(options[0]).toHaveTextContent('Match-beta'); + }); + + // Returning to "alpha" must re-trigger the fetch (cache wasn't poisoned). + const callsBeforeAlphaReturn = loadOptions.mock.calls.filter( + args => args[0] === 'alpha', + ).length; + await type('alpha'); + await waitFor(() => { + const callsAfter = loadOptions.mock.calls.filter( + args => args[0] === 'alpha', + ).length; + expect(callsAfter).toBeGreaterThan(callsBeforeAlphaReturn); + }); + await waitFor(async () => { + const options = await findAllSelectOptions(); + expect(options[0]).toHaveTextContent('Match-alpha'); + }); +}); + +test('keeps loading indicator while a newer request is in flight after a stale response is dropped', async () => { + // Regression for the P2 race: the `.finally` block that clears isLoading + // must not fire when a stale (dropped) response resolves while a newer + // request is still in flight. Otherwise the spinner disappears mid-search + // and the undebounced scroll-pagination handler can fire against stale + // totalCount before page 0 of the active search lands. + type OptionRow = { label: string; value: string | number }; + type PageResponse = { data: OptionRow[]; totalCount: number }; + // Initialized to no-op so the finally block can always call them, even if + // an assertion in the try throws before the corresponding mock ran. + let resolveAlpha: (value: PageResponse) => void = () => {}; + let resolveBeta: (value: PageResponse) => void = () => {}; + const page0Data: OptionRow[] = Array.from({ length: 10 }, (_, i) => ({ + label: `Option ${i}`, + value: i, + })); + const alphaData: OptionRow[] = [{ label: 'Match-alpha', value: 'va' }]; + const betaData: OptionRow[] = [{ label: 'Match-beta', value: 'vb' }]; + + const loadOptions = jest.fn((search: string) => { + if (search === '') { + return Promise.resolve({ + data: page0Data, + totalCount: 100, + }); + } + if (search === 'alpha') { + return new Promise(resolve => { + resolveAlpha = resolve; + }); + } + return new Promise(resolve => { + resolveBeta = resolve; + }); + }); + + const isSpinnerVisible = (): boolean => + Boolean(document.querySelector('.ant-select-arrow .ant-spin')); + + try { + render(); + await open(); + await waitFor(() => expect(loadOptions).toHaveBeenCalledWith('', 0, 10)); + + // Type 'alpha' — alpha fetch is held, loading should be true. + await type('alpha'); + await waitFor(() => + expect(loadOptions).toHaveBeenCalledWith('alpha', 0, 10), + ); + await waitFor(() => expect(isSpinnerVisible()).toBe(true)); + + // Type 'beta' — beta fetch is also held; both are in flight. + await type('beta'); + await waitFor(() => + expect(loadOptions).toHaveBeenCalledWith('beta', 0, 10), + ); + expect(isSpinnerVisible()).toBe(true); + + // Release the stale alpha response. It is dropped at the early-return + // (search !== inputValueRef.current), but the in-flight counter is still + // non-zero because beta is pending — spinner must stay visible. + resolveAlpha({ data: alphaData, totalCount: 1 }); + // Yield a microtask so alpha's .then/.finally runs, then re-assert. + await Promise.resolve(); + expect(isSpinnerVisible()).toBe(true); + + // Release beta. Now the in-flight counter drops to 0 and the spinner + // clears. + resolveBeta({ data: betaData, totalCount: 1 }); + await waitFor(() => expect(isSpinnerVisible()).toBe(false)); + const options = await findAllSelectOptions(); + expect(options).toHaveLength(1); + expect(options[0]).toHaveTextContent('Match-beta'); + } finally { + // Defensive: never leave a held promise that could hang a parallel worker + // if an assertion above threw. Promise resolve is idempotent. + resolveAlpha({ data: alphaData, totalCount: 1 }); + resolveBeta({ data: betaData, totalCount: 1 }); + } +}); + +test('re-shows search results when the same search term is repeated after a clear', async () => { + // Regression: a prior fix cached search responses' totalCount in + // fetchedQueries. After restore-on-clear had replaced selectOptions with + // the base list, re-typing a previously-resolved term would hit the cache + // short-circuit and leave selectOptions stale (empty / base-only). + const page0Data = Array.from({ length: 10 }, (_, i) => ({ + label: `Option ${i}`, + value: i, + })); + const alphaData = [{ label: 'Match-alpha', value: 'va' }]; + const loadOptions = jest.fn(async (search: string) => { + if (search === '') { + // totalCount > data.length so allValuesLoaded stays false and the + // search path is not bypassed by the "all loaded" short-circuit. + return { data: page0Data, totalCount: 100 }; + } + return { data: alphaData, totalCount: 1 }; + }); + + render(); + await open(); + await waitFor(() => expect(loadOptions).toHaveBeenCalledWith('', 0, 10)); + + await type('alpha'); + await waitFor(async () => { + const options = await findAllSelectOptions(); + expect(options).toHaveLength(1); + expect(options[0]).toHaveTextContent('Match-alpha'); + }); + + await type(''); + await waitFor(() => + expect(screen.queryByText('Match-alpha')).not.toBeInTheDocument(), + ); + + const callsBefore = loadOptions.mock.calls.filter( + args => args[0] === 'alpha', + ).length; + await type('alpha'); + await waitFor(() => { + const callsAfter = loadOptions.mock.calls.filter( + args => args[0] === 'alpha', + ).length; + expect(callsAfter).toBeGreaterThan(callsBefore); + }); + await waitFor(async () => { + const options = await findAllSelectOptions(); + expect(options).toHaveLength(1); + expect(options[0]).toHaveTextContent('Match-alpha'); + }); +}); + +test('appends page>1 results during an active search and discards them when search changes', async () => { + // Covers the production branch `else { mergeData(data) }` in fetchPage that + // fires when search is non-empty AND page > 0 — i.e. user scrolled within + // a multi-page search result. Switching to a new search must replace, not + // retain, the prior search's accumulated pages. + type OptionRow = { label: string; value: string | number }; + const pageSize = 5; + const aliceData: OptionRow[] = Array.from({ length: 5 }, (_, i) => ({ + label: `Alice-${i}`, + value: `a${i}`, + })); + const alicePage1: OptionRow[] = Array.from({ length: 3 }, (_, i) => ({ + label: `Alice-${i + 5}`, + value: `a${i + 5}`, + })); + const bobData: OptionRow[] = [{ label: 'Bob-0', value: 'b0' }]; + + const loadOptions = jest.fn( + async ( + search: string, + page: number, + ): Promise<{ + data: OptionRow[]; + totalCount: number; + }> => { + if (search === '') { + return { data: [], totalCount: 100 }; + } + if (search === 'alice') { + if (page === 0) return { data: aliceData, totalCount: 8 }; + return { data: alicePage1, totalCount: 8 }; + } + return { data: bobData, totalCount: 1 }; + }, + ); + + render( + , + ); + await open(); + + await type('alice'); + await waitFor(() => + expect(loadOptions).toHaveBeenCalledWith('alice', 0, pageSize), + ); + await waitFor(async () => { + const options = await findAllSelectOptions(); + expect(options).toHaveLength(5); + }); + // Wait for loading to finish so handlePagination's `!isLoading` gate is + // open before we fire scroll. + await waitFor(() => + expect(document.querySelector('.ant-select-arrow .ant-spin')).toBeNull(), + ); + + // Trigger pagination by dispatching a scroll event on the virtual-list + // scroll container. jsdom returns 0 for layout properties by default, so + // override the relevant ones before firing scroll. rc-virtual-list reads + // scrollTop via e.currentTarget in its onFallbackScroll handler, which + // then forwards to onPopupScroll (handlePagination here). + const holder = document.querySelector( + '.rc-virtual-list-holder', + ) as HTMLElement | null; + if (!holder) throw new Error('virtual-list holder not rendered'); + Object.defineProperty(holder, 'scrollHeight', { + configurable: true, + get: () => 1000, + }); + Object.defineProperty(holder, 'offsetHeight', { + configurable: true, + get: () => 200, + }); + Object.defineProperty(holder, 'clientHeight', { + configurable: true, + get: () => 200, + }); + Object.defineProperty(holder, 'scrollTop', { + configurable: true, + get: () => 900, + set: () => {}, + }); + fireEvent.scroll(holder); + + await waitFor(() => + expect(loadOptions).toHaveBeenCalledWith('alice', 1, pageSize), + ); + await waitFor(async () => { + const options = await findAllSelectOptions(); + // Page 0 (5) + page 1 (3) merged + expect(options).toHaveLength(8); + }); + + // Switching to a new search must replace the accumulated pages, not retain + // them. + await type('bob'); + await waitFor(() => + expect(loadOptions).toHaveBeenCalledWith('bob', 0, pageSize), + ); + await waitFor(async () => { + const options = await findAllSelectOptions(); + expect(options).toHaveLength(1); + expect(options[0]).toHaveTextContent('Bob-0'); + }); + expect(screen.queryByText('Alice-0')).not.toBeInTheDocument(); + expect(screen.queryByText('Alice-7')).not.toBeInTheDocument(); +}); + test('does not duplicate options when using numeric values', async () => { render( ()); + const initialOptionsRef = useRef(EMPTY_OPTIONS); + const inputValueRef = useRef(''); + // Counts fetches whose `.finally` has not yet run. Loading is cleared only + // when this drops to 0, so a stale response (which returns early without + // updating selectOptions) cannot flip the spinner off while a newer + // request is still pending. + const inFlightFetchesRef = useRef(0); const mappedMode = isSingleMode ? undefined : 'multiple'; const allowFetch = !fetchOnlyOnSearch || inputValue; const [maxTagCount, setMaxTagCount] = useState( @@ -183,6 +190,10 @@ const AsyncSelect = forwardRef( selectValueRef.current = selectValue; }, [selectValue]); + useEffect(() => { + inputValueRef.current = inputValue; + }, [inputValue]); + const sortSelectedFirst = useCallback( (a: AntdLabeledValue, b: AntdLabeledValue) => sortSelectedFirstHelper(a, b, selectValueRef.current), @@ -333,22 +344,78 @@ const AsyncSelect = forwardRef( setIsLoading(true); const fetchOptions = options as SelectOptionsPagePromise; + inFlightFetchesRef.current += 1; fetchOptions(search, page, pageSize) .then(({ data, totalCount }: SelectOptionsTypePage) => { - const mergedData = mergeData(data); - fetchedQueries.current.set(key, totalCount); - setTotalCount(totalCount); - if ( - !fetchOnlyOnSearch && - search === '' && - mergedData.length >= totalCount - ) { - setAllValuesLoaded(true); + // Drop responses whose search arg no longer matches the user's + // current input — otherwise a slow base fetch can land after a + // search fetch (or a stale debounced search after a clear) and + // re-pollute the dropdown via mergeData / search-replace. Search + // responses are never cached in fetchedQueries: the cache stores + // only totalCount, so a cache hit would short-circuit the fetch + // and leave selectOptions stale (e.g. after restore-on-clear). + // Re-issuing the search is cheap and correct. + const matchesCurrentSearch = inputValueRef.current === search; + if (search && !matchesCurrentSearch) { + return; + } + if (!search) { + // Accumulate base pages in a ref independent of selectOptions + // (during an active search, selectOptions holds search results + // and is not a safe accumulator). The accumulator is kept up + // to date even when this response landed during a search, so + // restore-on-clear has a complete snapshot. We don't sort here + // — restore-on-clear sorts a copy at consumption time, and the + // live selectOptions path below goes through mergeData which + // sorts there. Sorting here too would double the per-page sort + // cost on large cached option sets. + const dataValues = new Set(data.map(opt => opt.value)); + const accumulated = initialOptionsRef.current + .filter(opt => !dataValues.has(opt.value)) + .concat(data); + initialOptionsRef.current = accumulated; + if (!fetchOnlyOnSearch && accumulated.length >= totalCount) { + setAllValuesLoaded(true); + } + fetchedQueries.current.set(key, totalCount); + if (matchesCurrentSearch) { + // No active search — push to live selectOptions and update + // totalCount. When matchesCurrentSearch is false, the user + // is mid-search; leave the search's totalCount in place so + // pagination math stays correct. + mergeData(data); + setTotalCount(totalCount); + } + } else if (page === 0) { + // Replace cached options with server results; preserve + // optimistic isNewOption entries inserted by handleOnSearch + // so allowNewOptions users can still click the value they + // typed when the server returns no match. + setSelectOptions(prevOptions => { + const dataValues = new Set(data.map(opt => opt.value)); + const preservedNew = prevOptions.filter( + opt => opt.isNewOption && !dataValues.has(opt.value), + ); + return preservedNew + .concat(data) + .sort(sortComparatorForNoSearch); + }); + setTotalCount(totalCount); + } else { + // page > 0 during an active search — append normally. + mergeData(data); + setTotalCount(totalCount); } }) .catch(internalOnError) .finally(() => { - setIsLoading(false); + inFlightFetchesRef.current = Math.max( + 0, + inFlightFetchesRef.current - 1, + ); + if (inFlightFetchesRef.current === 0) { + setIsLoading(false); + } }); }, [ @@ -358,6 +425,7 @@ const AsyncSelect = forwardRef( internalOnError, options, pageSize, + sortComparatorForNoSearch, ], ); @@ -500,6 +568,7 @@ const AsyncSelect = forwardRef( fetchedQueries.current.clear(); setAllValuesLoaded(false); setSelectOptions(EMPTY_OPTIONS); + initialOptionsRef.current = EMPTY_OPTIONS; }, [options]); useEffect(() => { @@ -514,16 +583,36 @@ const AsyncSelect = forwardRef( [debouncedFetchPage], ); + const previousInputValue = usePrevious(inputValue, ''); useEffect(() => { if (loadingEnabled && allowFetch) { // trigger fetch every time inputValue changes if (inputValue) { debouncedFetchPage(inputValue, 0); } else { + // Cancel any pending debounced search fetch so it can't fire after + // we've already restored the base list. + debouncedFetchPage.cancel(); + // On returning to empty input after a search, restore the cached + // base options so the dropdown shows the original page-0 list + // instead of the stale search results. + if (previousInputValue && initialOptionsRef.current.length > 0) { + setSelectOptions( + [...initialOptionsRef.current].sort(sortComparatorForNoSearch), + ); + } fetchPage('', 0); } } - }, [loadingEnabled, fetchPage, allowFetch, inputValue, debouncedFetchPage]); + }, [ + loadingEnabled, + fetchPage, + allowFetch, + inputValue, + previousInputValue, + debouncedFetchPage, + sortComparatorForNoSearch, + ]); useEffect(() => { if (loading !== undefined && loading !== isLoading) { @@ -531,7 +620,11 @@ const AsyncSelect = forwardRef( } }, [isLoading, loading]); - const clearCache = () => fetchedQueries.current.clear(); + const clearCache = () => { + fetchedQueries.current.clear(); + initialOptionsRef.current = EMPTY_OPTIONS; + setAllValuesLoaded(false); + }; useImperativeHandle( ref, diff --git a/superset-frontend/packages/superset-ui-core/src/components/Select/utils.tsx b/superset-frontend/packages/superset-ui-core/src/components/Select/utils.tsx index 05f5483fe1f1..2f245cb9ab72 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/Select/utils.tsx +++ b/superset-frontend/packages/superset-ui-core/src/components/Select/utils.tsx @@ -211,6 +211,10 @@ export const handleFilterOptionHelper = ( return filterOption(search, option); } + if (filterOption === false) { + return true; + } + if (filterOption) { const searchValue = search.trim().toLowerCase(); if (optionFilterProps?.length) {