diff --git a/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts b/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts index 91f39f581c33..f4a6f9df4a4c 100644 --- a/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts @@ -127,21 +127,10 @@ describe('Nativefilters Sanity test', () => { .within(() => cy.get('input').type('wb_health_population{enter}', { force: true }), ); - // Add following step to avoid flaky enter value in line 177 - cy.get(nativeFilters.filtersPanel.inputDropdown) - .should('be.visible', { timeout: 20000 }) - .last() - .click(); - cy.get('.loading inline-centered css-101mkpk').should('not.exist'); - // hack for unclickable country_name - cy.wait(5000); - cy.get(nativeFilters.filtersPanel.filterInfoInput) - .last() - .should('be.visible', { timeout: 30000 }) - .click({ force: true }); - cy.get(nativeFilters.filtersPanel.filterInfoInput) + cy.get(`${nativeFilters.filtersPanel.filterInfoInput}:visible:last`) .last() + .focus() .type('country_name'); cy.get(nativeFilters.filtersPanel.inputDropdown) .should('be.visible', { timeout: 20000 }) diff --git a/superset-frontend/src/components/Select/Select.test.tsx b/superset-frontend/src/components/Select/Select.test.tsx index c9982e5ef982..c23f57d523d3 100644 --- a/superset-frontend/src/components/Select/Select.test.tsx +++ b/superset-frontend/src/components/Select/Select.test.tsx @@ -17,13 +17,7 @@ * under the License. */ import React from 'react'; -import { - render, - screen, - waitFor, - waitForElementToBeRemoved, - within, -} from 'spec/helpers/testing-library'; +import { render, screen, waitFor, within } from 'spec/helpers/testing-library'; import userEvent from '@testing-library/user-event'; import { Select } from 'src/components'; @@ -388,13 +382,6 @@ test('static - does not show "Loading..." when allowNewOptions is false and a ne expect(screen.queryByText(LOADING)).not.toBeInTheDocument(); }); -test('static - shows "Loading..." when allowNewOptions is true and a new option is entered', async () => { - render(); const option = OPTIONS[0].label; @@ -456,15 +443,6 @@ test('async - displays the loading indicator when opening', async () => { expect(screen.queryByText(LOADING)).not.toBeInTheDocument(); }); -test('async - displays the loading indicator while searching', async () => { - render(); const optionText = 'Emma'; @@ -587,24 +565,29 @@ test('async - sets a initial value in multiple mode', async () => { expect(values[1]).toHaveTextContent(OPTIONS[1].label); }); -test('async - searches for an item already loaded', async () => { +test('async - searches for matches in both loaded and unloaded pages', async () => { render(); - const search = 'Ashfaq'; + const mock = jest.fn(loadOptions); + render(); await open(); expect(mock).toHaveBeenCalledTimes(1); - await type(search); - expect(await findSelectOption(search)).toBeInTheDocument(); + await type('or'); + + // `George` is on the first page so when it appears the API has not been called again + expect(await findSelectOption('George')).toBeInTheDocument(); + expect(mock).toHaveBeenCalledTimes(1); + + // `Igor` is on the second paged API request + expect(await findSelectOption('Igor')).toBeInTheDocument(); expect(mock).toHaveBeenCalledTimes(2); }); diff --git a/superset-frontend/src/components/Select/Select.tsx b/superset-frontend/src/components/Select/Select.tsx index a36a0d569aeb..12b481bcc0f7 100644 --- a/superset-frontend/src/components/Select/Select.tsx +++ b/superset-frontend/src/components/Select/Select.tsx @@ -20,7 +20,6 @@ import React, { ReactElement, ReactNode, RefObject, - KeyboardEvent, UIEvent, useEffect, useMemo, @@ -40,7 +39,8 @@ import { isEqual } from 'lodash'; import { Spin } from 'antd'; import Icons from 'src/components/Icons'; import { getClientErrorObject } from 'src/utils/getClientErrorObject'; -import { hasOption } from './utils'; +import { SLOW_DEBOUNCE } from 'src/constants'; +import { hasOption, hasOptionIgnoreCase } from './utils'; const { Option } = AntdSelect; @@ -58,17 +58,13 @@ type PickedSelectProps = Pick< | 'onChange' | 'onClear' | 'onFocus' + | 'onBlur' | 'placeholder' | 'showSearch' | 'value' >; -type OptionsProps = Exclude; - -export interface OptionsType extends Omit { - label?: string; - customLabel?: ReactNode; -} +export type OptionsType = Exclude; export type OptionsTypePage = { data: OptionsType; @@ -220,7 +216,6 @@ const StyledLoadingText = styled.div` const MAX_TAG_COUNT = 4; const TOKEN_SEPARATORS = [',', '\n', '\t', ';']; -const DEBOUNCE_TIMEOUT = 500; const DEFAULT_PAGE_SIZE = 100; const EMPTY_OPTIONS: OptionsType = []; @@ -252,6 +247,9 @@ export const propertyComparator = return (a[property] as number) - (b[property] as number); }; +const getQueryCacheKey = (value: string, page: number, pageSize: number) => + `${value};${page};${pageSize}`; + /** * This component is a customized version of the Antdesign 4.X Select component * https://ant.design/components/select/. @@ -305,7 +303,6 @@ const Select = ({ const [selectValue, setSelectValue] = useState(value); const [searchedValue, setSearchedValue] = useState(''); const [isLoading, setIsLoading] = useState(loading); - const [isTyping, setIsTyping] = useState(false); const [error, setError] = useState(''); const [isDropdownVisible, setIsDropdownVisible] = useState(false); const [page, setPage] = useState(0); @@ -318,6 +315,7 @@ const Select = ({ : allowNewOptions ? 'tags' : 'multiple'; + const allowFetch = !fetchOnlyOnSearch || searchedValue; // TODO: Don't assume that isAsync is always labelInValue const handleTopOptions = useCallback( @@ -478,18 +476,16 @@ const Select = ({ ); const handlePaginatedFetch = useMemo( - () => (value: string, page: number, pageSize: number) => { + () => (value: string, page: number) => { if (allValuesLoaded) { setIsLoading(false); - setIsTyping(false); return; } - const key = `${value};${page};${pageSize}`; + const key = getQueryCacheKey(value, page, pageSize); const cachedCount = fetchedQueries.current.get(key); - if (cachedCount) { + if (cachedCount !== undefined) { setTotalCount(cachedCount); setIsLoading(false); - setIsTyping(false); return; } setIsLoading(true); @@ -510,40 +506,57 @@ const Select = ({ .catch(internalOnError) .finally(() => { setIsLoading(false); - setIsTyping(false); }); }, - [allValuesLoaded, fetchOnlyOnSearch, handleData, internalOnError, options], + [ + allValuesLoaded, + fetchOnlyOnSearch, + handleData, + internalOnError, + options, + pageSize, + ], ); - const handleOnSearch = useMemo( + const debouncedHandleSearch = useMemo( () => debounce((search: string) => { - const searchValue = search.trim(); - if (allowNewOptions && isSingleMode) { - const newOption = searchValue && - !hasOption(searchValue, selectOptions) && { - label: searchValue, - value: searchValue, - }; - const newOptions = newOption - ? [ - newOption, - ...selectOptions.filter(opt => opt.value !== searchedValue), - ] - : [...selectOptions.filter(opt => opt.value !== searchedValue)]; - - setSelectOptions(newOptions); - } - - if (!searchValue || searchValue === searchedValue) { - setIsTyping(false); - } - setSearchedValue(searchValue); - }, DEBOUNCE_TIMEOUT), - [allowNewOptions, isSingleMode, searchedValue, selectOptions], + // async search will be triggered in handlePaginatedFetch + setSearchedValue(search); + }, SLOW_DEBOUNCE), + [], ); + const handleOnSearch = (search: string) => { + const searchValue = search.trim(); + if (allowNewOptions && isSingleMode) { + const newOption = searchValue && + !hasOptionIgnoreCase(searchValue, selectOptions) && { + label: searchValue, + value: searchValue, + isNewOption: true, + }; + const cleanSelectOptions = selectOptions.filter( + opt => !opt.isNewOption || hasOption(opt.value, selectValue), + ); + const newOptions = newOption + ? [newOption, ...cleanSelectOptions] + : cleanSelectOptions; + setSelectOptions(newOptions); + } + if ( + isAsync && + !allValuesLoaded && + loadingEnabled && + !fetchedQueries.current.has(getQueryCacheKey(searchValue, 0, pageSize)) + ) { + // if fetch only on search but search value is empty, then should not be + // in loading state + setIsLoading(!(fetchOnlyOnSearch && !searchValue)); + } + return debouncedHandleSearch(search); + }; + const handlePagination = (e: UIEvent) => { const vScroll = e.currentTarget; const thresholdReached = @@ -552,7 +565,7 @@ const Select = ({ if (!isLoading && isAsync && hasMoreData && thresholdReached) { const newPage = page + 1; - handlePaginatedFetch(searchedValue, newPage, pageSize); + handlePaginatedFetch(searchedValue, newPage); setPage(newPage); } }; @@ -581,8 +594,21 @@ const Select = ({ const handleOnDropdownVisibleChange = (isDropdownVisible: boolean) => { setIsDropdownVisible(isDropdownVisible); - if (isAsync && !loadingEnabled) { - setLoadingEnabled(true); + if (isAsync) { + // loading is enabled when dropdown is open, + // disabled when dropdown is closed + if (loadingEnabled !== isDropdownVisible) { + setLoadingEnabled(isDropdownVisible); + } + // when closing dropdown, always reset loading state + if (!isDropdownVisible && isLoading) { + // delay is for the animation of closing the dropdown + // so the dropdown doesn't flash between "Loading..." and "No data" + // before closing. + setTimeout(() => { + setIsLoading(false); + }, 250); + } } // multiple or tags mode keep the dropdown visible while selecting options @@ -598,19 +624,15 @@ const Select = ({ if (!isDropdownVisible) { originNode.ref?.current?.scrollTo({ top: 0 }); } - if ((isLoading && selectOptions.length === 0) || isTyping) { + if (isLoading && selectOptions.length === 0) { return {t('Loading...')}; } return error ? : originNode; }; - const onInputKeyDown = (event: KeyboardEvent) => { - if (event.key.length === 1 && isAsync && !isTyping) { - setIsTyping(true); - } - }; - - const SuffixIcon = () => { + // use a function instead of component since every rerender of the + // Select component will create a new component + const getSuffixIcon = () => { if (isLoading) { return ; } @@ -672,22 +694,24 @@ const Select = ({ }, [labelInValue, isAsync, selectValue]); // Stop the invocation of the debounced function after unmounting - useEffect(() => () => handleOnSearch.cancel(), [handleOnSearch]); + useEffect( + () => () => { + debouncedHandleSearch.cancel(); + }, + [debouncedHandleSearch], + ); useEffect(() => { - const allowFetch = !fetchOnlyOnSearch || searchedValue; if (isAsync && loadingEnabled && allowFetch) { - const page = 0; - handlePaginatedFetch(searchedValue, page, pageSize); - setPage(page); + handlePaginatedFetch(searchedValue, 0); + setPage(0); } }, [ isAsync, searchedValue, - pageSize, handlePaginatedFetch, loadingEnabled, - fetchOnlyOnSearch, + allowFetch, ]); useEffect(() => { @@ -713,16 +737,9 @@ const Select = ({ labelInValue={isAsync || labelInValue} maxTagCount={MAX_TAG_COUNT} mode={mappedMode} - notFoundContent={ - allowNewOptions && !fetchOnlyOnSearch ? ( - {t('Loading...')} - ) : ( - notFoundContent - ) - } + notFoundContent={isLoading ? t('Loading...') : notFoundContent} onDeselect={handleOnDeselect} onDropdownVisibleChange={handleOnDropdownVisibleChange} - onInputKeyDown={onInputKeyDown} onPopupScroll={isAsync ? handlePagination : undefined} onSearch={shouldShowSearch ? handleOnSearch : undefined} onSelect={handleOnSelect} @@ -734,7 +751,7 @@ const Select = ({ showArrow tokenSeparators={TOKEN_SEPARATORS} value={selectValue} - suffixIcon={} + suffixIcon={getSuffixIcon()} menuItemSelectedIcon={ invertSelection ? ( diff --git a/superset-frontend/src/components/Select/utils.ts b/superset-frontend/src/components/Select/utils.ts index 71a904520591..1a3e3ca3b340 100644 --- a/superset-frontend/src/components/Select/utils.ts +++ b/superset-frontend/src/components/Select/utils.ts @@ -1,3 +1,4 @@ +import { ensureIsArray } from '@superset-ui/core'; /** * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file @@ -60,7 +61,19 @@ export function findValue( return (Array.isArray(value) ? value : [value]).map(find); } -export function hasOption(search: string, options: AntdOptionsType) { +export function hasOption( + value: VT, + options?: VT | VT[] | { value: VT } | { value: VT }[], +) { + const optionsArray = ensureIsArray(options); + return ( + optionsArray.find(x => + typeof x === 'object' ? x.value === value : x === value, + ) !== undefined + ); +} + +export function hasOptionIgnoreCase(search: string, options: AntdOptionsType) { const searchOption = search.trim().toLowerCase(); return options.find(opt => { const { label, value } = opt;