From 6a16e7dca4ddff46052302ac730c0a11e69c8f67 Mon Sep 17 00:00:00 2001 From: kasiazjc Date: Wed, 20 May 2026 14:41:18 +0000 Subject: [PATCH] refactor(listview): CompactSelectPanel reuses Select/AsyncSelect Replace custom option rendering, debounced search, race-condition guards, and loading states with Select/AsyncSelect from @superset-ui/core/components. The trigger is hidden via a zero-height wrapper; the dropdown renders inside a container div via getPopupContainer, giving the same visual as Explore's select dropdowns. Also exposes the 'open' prop in AntdExposedProps so Select/AsyncSelect can be controlled externally. Removes ~130 lines of reimplemented debounce/race-condition/ filter logic and eliminates the outline hover hack. Co-Authored-By: Claude Sonnet 4.6 --- .../src/components/Select/types.ts | 1 + .../Filters/CompactSelectPanel.test.tsx | 315 ++++++++--------- .../ListView/Filters/CompactSelectPanel.tsx | 318 ++++++------------ 3 files changed, 229 insertions(+), 405 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/components/Select/types.ts b/superset-frontend/packages/superset-ui-core/src/components/Select/types.ts index 7d38dba9d81..ddc32b110ed 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/Select/types.ts +++ b/superset-frontend/packages/superset-ui-core/src/components/Select/types.ts @@ -72,6 +72,7 @@ export type AntdExposedProps = Pick< | 'getPopupContainer' | 'menuItemSelectedIcon' | 'dropdownAlign' + | 'open' >; export type SelectOptionsType = Exclude; diff --git a/superset-frontend/src/components/ListView/Filters/CompactSelectPanel.test.tsx b/superset-frontend/src/components/ListView/Filters/CompactSelectPanel.test.tsx index a41706f99ae..1adb2c81632 100644 --- a/superset-frontend/src/components/ListView/Filters/CompactSelectPanel.test.tsx +++ b/superset-frontend/src/components/ListView/Filters/CompactSelectPanel.test.tsx @@ -17,67 +17,87 @@ * under the License. */ import { createRef, act } from 'react'; -import { render, screen, waitFor } from 'spec/helpers/testing-library'; -import userEvent from '@testing-library/user-event'; +import { render, screen } from 'spec/helpers/testing-library'; import CompactSelectPanel from './CompactSelectPanel'; import type { FilterHandler } from './types'; -const SMALL_SELECTS = [ +const SELECTS = [ { label: 'Alice', value: 1 }, { label: 'Bob', value: 2 }, { label: 'Charlie', value: 3 }, ]; -const LARGE_SELECTS = [ - { label: 'Alice', value: 1 }, - { label: 'Bob', value: 2 }, - { label: 'Charlie', value: 3 }, - { label: 'David', value: 4 }, - { label: 'Eve', value: 5 }, - { label: 'Frank', value: 6 }, - { label: 'Grace', value: 7 }, -]; +jest.mock('@superset-ui/core/components', () => ({ + Select: jest.fn(({ onChange, onOpenChange, value, open, ariaLabel }: any) => ( +
+ + + + {value && {String(value.value)}} +
+ )), + AsyncSelect: jest.fn(({ onChange, onOpenChange, open, ariaLabel }: any) => ( +
+ + +
+ )), +})); beforeEach(() => { jest.clearAllMocks(); }); -test('renders options from selects prop', () => { +test('renders Select when selects prop is provided', () => { render( , ); - expect(screen.getByText('Alice')).toBeInTheDocument(); - expect(screen.getByText('Bob')).toBeInTheDocument(); - expect(screen.getByText('Charlie')).toBeInTheDocument(); + expect(screen.getByTestId('mock-select')).toBeInTheDocument(); }); -test('hides search input when selects.length is 6 or fewer', () => { - render( - , - ); - expect(screen.queryByPlaceholderText('Search')).not.toBeInTheDocument(); -}); - -test('shows search input when selects.length exceeds 6', () => { - render( - , - ); - expect(screen.getByPlaceholderText('Search')).toBeInTheDocument(); -}); - -test('shows search input when fetchSelects is provided', () => { +test('renders AsyncSelect when fetchSelects prop is provided', () => { const fetchSelects = jest.fn().mockResolvedValue({ data: [], totalCount: 0 }); render( { onSelect={jest.fn()} />, ); - expect(screen.getByPlaceholderText('Search')).toBeInTheDocument(); + expect(screen.getByTestId('mock-async-select')).toBeInTheDocument(); }); -test('filters static options by search term', async () => { +test('passes isOpen as open prop to Select', () => { render( , ); - await userEvent.type(screen.getByPlaceholderText('Search'), 'ali'); - expect(screen.getByText('Alice')).toBeInTheDocument(); - expect(screen.queryByText('Bob')).not.toBeInTheDocument(); + expect(screen.getByTestId('mock-select')).toHaveAttribute( + 'data-open', + 'true', + ); }); -test('calls onSelect with normalized option when an option is clicked', async () => { +test('calls onSelect with normalized option when onChange fires', () => { const onSelect = jest.fn(); render( , ); - await userEvent.click(screen.getByText('Alice')); + screen.getByTestId('select-option-alice').click(); expect(onSelect).toHaveBeenCalledWith({ label: 'Alice', value: 1 }, false); }); -test('calls onSelect with undefined when same option is clicked twice (deselect)', async () => { +test('calls onSelect(undefined, true) when onChange fires with undefined', () => { const onSelect = jest.fn(); + const { Select: MockSelect } = jest.requireMock( + '@superset-ui/core/components', + ) as any; + MockSelect.mockImplementationOnce(({ onChange }: any) => ( + + )); render( , ); - await userEvent.click(screen.getByText('Alice')); + screen.getByTestId('trigger-clear').click(); expect(onSelect).toHaveBeenCalledWith(undefined, true); }); -test('shows checkmark icon on selected option', () => { - render( - , - ); - const aliceOption = screen - .getByText('Alice') - .closest('[role="option"]') as HTMLElement; - expect(aliceOption).toHaveAttribute('aria-selected', 'true'); -}); - -test('unselected options have aria-selected false', () => { - render( - , - ); - const bobOption = screen - .getByText('Bob') - .closest('[role="option"]') as HTMLElement; - expect(bobOption).toHaveAttribute('aria-selected', 'false'); -}); - -test('calls onClose after a selection is made', async () => { +test('calls onClose after onChange fires', () => { const onClose = jest.fn(); render( , ); - await userEvent.click(screen.getByText('Alice')); + screen.getByTestId('select-option-alice').click(); + expect(onClose).toHaveBeenCalledTimes(1); +}); + +test('calls onClose when onOpenChange fires false', () => { + const onClose = jest.fn(); + render( + , + ); + screen.getByTestId('trigger-close').click(); expect(onClose).toHaveBeenCalledTimes(1); }); @@ -176,154 +196,81 @@ test('clearFilter via ref resets selection and calls onSelect(undefined, true)', render( , ); - expect(screen.getByText('Alice').closest('[role="option"]')).toHaveAttribute( - 'aria-selected', - 'true', - ); - act(() => { ref.current?.clearFilter(); }); - expect(onSelect).toHaveBeenCalledWith(undefined, true); - expect(screen.getByText('Alice').closest('[role="option"]')).toHaveAttribute( - 'aria-selected', - 'false', - ); }); -test('shows Loading text when loading prop is true', () => { - render( +test('syncs selected state when external value prop changes', () => { + const { rerender } = render( , ); - expect(screen.getByText('Loading...')).toBeInTheDocument(); -}); + expect(screen.getByTestId('selected-value')).toHaveTextContent('1'); -test('shows No results when displayOptions is empty', () => { - render( - , - ); - expect(screen.getByText('No results')).toBeInTheDocument(); -}); - -test('renders options list with listbox role and accessible label', () => { - render( + rerender( , ); - const listbox = screen.getByRole('listbox'); - expect(listbox).toBeInTheDocument(); - expect(listbox).toHaveAttribute('aria-label', 'Filter options'); + expect(screen.queryByTestId('selected-value')).not.toBeInTheDocument(); }); -test('option items have option role', () => { - render( - , - ); - const options = screen.getAllByRole('option'); - expect(options).toHaveLength(3); -}); - -test('fetches and displays remote options via fetchSelects on mount', async () => { - const fetchSelects = jest.fn().mockResolvedValue({ - data: [{ label: 'Remote User', value: 99 }], - totalCount: 1, - }); - render( - , - ); - expect(screen.getByText('Loading...')).toBeInTheDocument(); - await waitFor(() => { - expect(screen.getByText('Remote User')).toBeInTheDocument(); - }); - expect(fetchSelects).toHaveBeenCalledWith('', 0, 50); -}); - -test('shows No results when fetchSelects returns empty data', async () => { +test('passes isOpen as open prop to AsyncSelect', () => { const fetchSelects = jest.fn().mockResolvedValue({ data: [], totalCount: 0 }); render( , ); - await waitFor(() => { - expect(screen.getByText('No results')).toBeInTheDocument(); - }); + expect(screen.getByTestId('mock-async-select')).toHaveAttribute( + 'data-open', + 'true', + ); }); -test('shows No results when fetchSelects rejects', async () => { - const fetchSelects = jest.fn().mockRejectedValue(new Error('network error')); +test('calls onClose when AsyncSelect onOpenChange fires false', () => { + const onClose = jest.fn(); + const fetchSelects = jest.fn().mockResolvedValue({ data: [], totalCount: 0 }); render( , ); - await waitFor(() => { - expect(screen.getByText('No results')).toBeInTheDocument(); - }); + screen.getByTestId('async-trigger-close').click(); + expect(onClose).toHaveBeenCalledTimes(1); }); -test('selects option via keyboard Enter key', async () => { +test('calls onSelect on AsyncSelect onChange', () => { const onSelect = jest.fn(); + const fetchSelects = jest.fn().mockResolvedValue({ data: [], totalCount: 0 }); render( , ); - const aliceOption = screen.getByText('Alice').closest('[role="option"]')!; - await userEvent.type(aliceOption, '{Enter}'); - expect(onSelect).toHaveBeenCalledWith({ label: 'Alice', value: 1 }, false); -}); - -test('syncs selected state when external value prop changes', () => { - const { rerender } = render( - , - ); - expect(screen.getByText('Alice').closest('[role="option"]')).toHaveAttribute( - 'aria-selected', - 'true', - ); - - rerender( - , - ); - expect(screen.getByText('Alice').closest('[role="option"]')).toHaveAttribute( - 'aria-selected', - 'false', + screen.getByTestId('async-option-remote').click(); + expect(onSelect).toHaveBeenCalledWith( + { label: 'Remote User', value: 99 }, + false, ); }); diff --git a/superset-frontend/src/components/ListView/Filters/CompactSelectPanel.tsx b/superset-frontend/src/components/ListView/Filters/CompactSelectPanel.tsx index a35382dda70..1ca20cc42b9 100644 --- a/superset-frontend/src/components/ListView/Filters/CompactSelectPanel.tsx +++ b/superset-frontend/src/components/ListView/Filters/CompactSelectPanel.tsx @@ -18,6 +18,7 @@ */ import { forwardRef, + useCallback, useImperativeHandle, useMemo, useRef, @@ -25,15 +26,9 @@ import { useEffect, type RefObject, } from 'react'; -import { debounce } from 'lodash'; import { t } from '@apache-superset/core/translation'; -import { useTheme, styled, css } from '@apache-superset/core/theme'; -import { - Icons, - Input, - Constants, - type InputRef, -} from '@superset-ui/core/components'; +import { styled } from '@apache-superset/core/theme'; +import { Select, AsyncSelect } from '@superset-ui/core/components'; import type { SelectOption, ListViewFilter as Filter } from '../types'; import type { FilterHandler } from './types'; @@ -43,80 +38,27 @@ interface CompactSelectPanelProps { value?: SelectOption; onSelect: (option: SelectOption | undefined, isClear?: boolean) => void; onClose?: () => void; - /** Injected by CompactFilterTrigger via cloneElement — true when dropdown is open */ + /** Injected by CompactFilterTrigger via cloneElement — true when the outer dropdown is open */ isOpen?: boolean; /** External loading state from filter config */ loading?: boolean; } -const PanelContainer = styled.div` - ${({ theme }) => css` - min-width: 220px; - max-width: 320px; - max-height: 320px; - display: flex; - flex-direction: column; - border-radius: ${theme.borderRadiusLG}px; - overflow: hidden; - background: ${theme.colorBgElevated}; - box-shadow: ${theme.boxShadowSecondary}; - padding: ${theme.paddingXXS}px 0; - `} +/** + * Shows only the dropdown portion of Select/AsyncSelect by hiding the trigger + * via a zero-height wrapper and rendering the dropdown inside a container div + * via getPopupContainer. This gives the same visual as Explore's select dropdowns. + */ +const Container = styled.div` + position: relative; `; -const SearchRow = styled.div` - ${({ theme }) => css` - padding: 0 ${theme.sizeUnit * 2}px ${theme.paddingXXS}px; - `} -`; - -const OptionList = styled.ul` - ${({ theme }) => css` - margin: 0; - padding: 0; - overflow-y: auto; - flex: 1; - list-style: none; - `} -`; - -const OptionItem = styled.li<{ $active: boolean }>` - ${({ theme, $active }) => css` - display: flex; - align-items: center; - justify-content: space-between; - padding: 5px ${theme.sizeUnit * 3}px; - cursor: pointer; - font-size: ${theme.fontSize}px; - color: ${theme.colorText}; - border-radius: ${theme.borderRadiusSM}px; - background: ${$active ? theme.colorPrimaryBg : 'transparent'}; - transition: background 0.15s; - - &:hover { - background: ${$active - ? theme.colorPrimaryBgHover - : theme.colorFillTertiary}; - outline: 2px solid ${theme.colorPrimary}; - outline-offset: -2px; - } - `} -`; - -const OptionLabel = styled.span` +const HiddenWrapper = styled.div` + position: absolute; + height: 0; + width: 100%; overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; - max-width: 240px; -`; - -const StatusText = styled.div` - ${({ theme }) => css` - padding: ${theme.sizeUnit * 2}px ${theme.sizeUnit * 3}px; - text-align: center; - color: ${theme.colorTextDisabled}; - font-size: ${theme.fontSizeSM}px; - `} + pointer-events: none; `; function CompactSelectPanel( @@ -127,179 +69,113 @@ function CompactSelectPanel( onSelect, onClose, isOpen, - loading: externalLoading, + loading, }: CompactSelectPanelProps, ref: RefObject, ) { - const theme = useTheme(); - const inputRef = useRef(null); - const [search, setSearch] = useState(''); - const [debouncedSearch, setDebouncedSearch] = useState(''); - const [remoteOptions, setRemoteOptions] = useState([]); - const [internalLoading, setInternalLoading] = useState(false); + const containerRef = useRef(null); const [selectedOption, setSelectedOption] = useState< SelectOption | undefined >(value); - const isLoading = externalLoading || internalLoading; - - const debouncedSetSearch = useMemo( - () => debounce(setDebouncedSearch, Constants.FAST_DEBOUNCE), - [], - ); - - useEffect( - () => () => { - debouncedSetSearch.cancel(); - }, - [debouncedSetSearch], - ); - - // Sync selected state when external value changes (e.g. clearFilters called from parent) + // Sync when external value changes (e.g. clearFilters from parent) useEffect(() => { setSelectedOption(value); }, [value]); - // Focus search input when dropdown opens; reset search when it closes - useEffect(() => { - let timeoutId: ReturnType; - if (isOpen) { - timeoutId = setTimeout(() => { - inputRef.current?.input?.focus({ preventScroll: true }); - }, 100); - } else { - setSearch(''); - setDebouncedSearch(''); - } - return () => { - if (timeoutId) clearTimeout(timeoutId); - }; - }, [isOpen]); - - // Fetch remote options when debounced search changes - useEffect(() => { - if (!fetchSelects) return; - let cancelled = false; - setInternalLoading(true); - fetchSelects(debouncedSearch, 0, 50) - .then(result => { - if (!cancelled) setRemoteOptions(result?.data ?? []); - }) - .catch(() => { - if (!cancelled) setRemoteOptions([]); - }) - .finally(() => { - if (!cancelled) setInternalLoading(false); - }); - return () => { - cancelled = true; - }; - }, [debouncedSearch, fetchSelects]); - useImperativeHandle(ref, () => ({ clearFilter: () => { setSelectedOption(undefined); - setSearch(''); - setDebouncedSearch(''); onSelect(undefined, true); }, })); - const displayOptions = ( - fetchSelects - ? remoteOptions - : selects.filter(o => { - const label = typeof o.label === 'string' ? o.label : String(o.value); - return label.toLowerCase().includes(search.toLowerCase()); - }) - ).filter(o => o != null); + const handleChange = useCallback( + (selected: SelectOption | undefined) => { + // Normalize to plain {label: string, value} to avoid circular-ref errors + // when emotion-styled ReactNode labels are URL-serialized. + const next = selected + ? { + label: + typeof selected.label === 'string' + ? selected.label + : String(selected.value ?? ''), + value: selected.value, + } + : undefined; + setSelectedOption(next); + onSelect(next, !selected); + onClose?.(); + }, + [onSelect, onClose], + ); - // Show search for async selects or large static lists - const showSearch = !!fetchSelects || selects.length > 6; + const handleOpenChange = useCallback( + (visible: boolean) => { + if (!visible) onClose?.(); + }, + [onClose], + ); - // displayText is the actual rendered text of the clicked list item, captured - // from the DOM via e.currentTarget.textContent. This is more reliable than - // reading opt.label, which may be a styled ReactNode (e.g. for owner options) - // rather than a plain string — causing tooltip to show the raw value instead. - const handleSelect = (opt: SelectOption, displayText?: string) => { - const isDeselect = selectedOption?.value === opt.value; - // Normalize to a plain object so the value can be safely serialized to - // URL query params without circular-reference errors from emotion metadata - // on styled ReactNode labels. - const next = isDeselect - ? undefined - : { - label: - displayText || - (typeof opt.label === 'string' ? opt.label : String(opt.value ?? '')), - value: opt.value, - }; - setSelectedOption(next); - onSelect(next, isDeselect); - onClose?.(); - }; + const getPopupContainer = useCallback( + () => containerRef.current ?? document.body, + [], + ); + + const fetchAndFormat = useMemo( + () => + fetchSelects + ? async (inputValue: string, page: number, pageSize: number) => { + const result = await fetchSelects(inputValue, page, pageSize); + return { + data: result?.data ?? [], + totalCount: result?.totalCount ?? 0, + }; + } + : undefined, + [fetchSelects], + ); + + const placeholder = t('Search'); return ( - - {showSearch && ( - - - } - placeholder={t('Search')} - value={search} - onChange={e => { - setSearch(e.target.value); - debouncedSetSearch(e.target.value); - }} + + + {fetchSelects ? ( + - - )} - - {isLoading ? ( - {t('Loading...')} - ) : displayOptions.length === 0 ? ( - {t('No results')} ) : ( - displayOptions.map(opt => { - const isActive = selectedOption?.value === opt.value; - const getDisplayText = (el: HTMLElement) => - el.textContent?.trim() || undefined; - return ( - handleSelect(opt, getDisplayText(e.currentTarget))} - onKeyDown={e => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - handleSelect(opt, getDisplayText(e.currentTarget)); - } - }} - > - {opt.label} - {isActive && ( - - )} - - ); - }) +