diff --git a/.changeset/filter-picker-performance.md b/.changeset/filter-picker-performance.md new file mode 100644 index 000000000..e64481137 --- /dev/null +++ b/.changeset/filter-picker-performance.md @@ -0,0 +1,5 @@ +--- +'@cube-dev/ui-kit': patch +--- + +Improve `FilterPicker` and `Picker` performance: fewer redundant re-renders, memoized label and key lookups, trigger width measured only when the popover opens, and a controlled popover state so the trigger subtree reconciles normally. diff --git a/src/components/fields/FilterPicker/FilterPicker.tsx b/src/components/fields/FilterPicker/FilterPicker.tsx index 6343456ef..62508c233 100644 --- a/src/components/fields/FilterPicker/FilterPicker.tsx +++ b/src/components/fields/FilterPicker/FilterPicker.tsx @@ -17,7 +17,6 @@ import { ReactElement, ReactNode, RefObject, - useCallback, useEffect, useMemo, useRef, @@ -31,8 +30,8 @@ import { useWarn } from '../../../_internal/hooks/use-warn'; import { CloseIcon, DirectionIcon, LoadingIcon } from '../../../icons'; import { useProviderProps } from '../../../provider'; import { generateRandomId } from '../../../utils/random'; -import { mergeProps } from '../../../utils/react'; import { useEventBus } from '../../../utils/react/useEventBus'; +import { processSelectionArray } from '../../../utils/selection'; import { extractStyles } from '../../../utils/styles'; import { CubeItemButtonProps, ItemAction, ItemButton } from '../../actions'; import { CubeItemProps } from '../../content/Item'; @@ -47,7 +46,6 @@ import { ListBox } from '../ListBox'; import type { FieldBaseProps } from '../../../shared'; -// Define interface for items that can have keys interface ItemWithKey { key?: string | number; id?: string | number; @@ -170,7 +168,6 @@ export const FilterPicker = forwardRef(function FilterPicker( fieldProps.onSelectionChange = (key: Key | null | 'all' | Key[]) => { if (props.selectionMode === 'multiple') { - // Handle "all" selection and array selections if (key === 'all') { onChange('all'); } else { @@ -267,20 +264,15 @@ export const FilterPicker = forwardRef(function FilterPicker( ...otherProps } = props; - // Track if sortSelectedToTop was explicitly provided const sortSelectedToTopExplicit = sortSelectedToTopProp !== undefined; - // Default to true if items are provided, false otherwise const sortSelectedToTop = sortSelectedToTopProp ?? (items ? true : false); styles = extractStyles(otherProps, PROP_STYLES, styles); - // Generate a unique ID for this FilterPicker instance const filterPickerId = useMemo(() => generateRandomId(), []); - // Get event bus for menu synchronization const { emit, on } = useEventBus(); - // Warn if isCheckable is false in single selection mode useWarn(isCheckable === false && selectionMode === 'single', { key: ['filterpicker-checkable-single-mode'], args: [ @@ -288,6 +280,13 @@ export const FilterPicker = forwardRef(function FilterPicker( ], }); + useWarn(sortSelectedToTopExplicit && sortSelectedToTop && !items, { + key: ['filterpicker-sort-selected-to-top-children'], + args: [ + 'FilterPicker: sortSelectedToTop only works with the items prop. Sorting will be skipped when using JSX children.', + ], + }); + // Internal selection state (uncontrolled scenario) const [internalSelectedKey, setInternalSelectedKey] = useState( defaultSelectedKey ?? null, @@ -296,19 +295,13 @@ export const FilterPicker = forwardRef(function FilterPicker( 'all' | Key[] >(defaultSelectedKeys ?? []); - // Track popover open/close and capture children order for session + // Popover state — used as controlled prop for DialogTrigger const [isPopoverOpen, setIsPopoverOpen] = useState(false); - // Cache for sorted items array when using `items` prop const cachedItemsOrder = useRef(null); const triggerRef = useRef>(null); + // Measured lazily on popover open instead of on every render + const triggerWidthRef = useRef(undefined); - // --------------------------------------------------------------------------- - // Invalidate cached sorting whenever the available options change. - // This ensures newly provided options are displayed and properly sorted on - // the next popover open instead of re-using a stale order from a previous - // session (which caused only the previously selected options to be rendered - // or the list to appear unsorted). - // --------------------------------------------------------------------------- useEffect(() => { cachedItemsOrder.current = null; }, [items]); @@ -323,140 +316,72 @@ export const FilterPicker = forwardRef(function FilterPicker( ? selectedKeys : internalSelectedKeys; - // Create a local collection for label extraction only (not for rendering) - // This gives us immediate access to textValue without waiting for FilterListBox + // Collection for label extraction (shared with FilterListBox via _internalCollection) const localCollectionState = useListState({ children: children as any, items: items as any, - selectionMode: 'none' as any, // We don't need selection management + selectionMode: 'none' as any, }); - // --------------------------------------------------------------------------- - // Map user-provided keys to collection keys using the local collection. - // The collection handles key normalization internally, so we use direct - // string comparison. - // --------------------------------------------------------------------------- - - const findCollectionKey = useCallback( - (lookup: Key): Key => { - if (lookup == null) return lookup; - - // Direct comparison - collection handles normalization internally - for (const item of localCollectionState.collection) { - if (String(item.key) === String(lookup)) { - return item.key; + // Build Maps for O(1) label and key lookups from the collection + const { labelMap, keyMap } = useMemo(() => { + const lm = new Map(); + const km = new Map(); + + const traverse = (nodes: Iterable) => { + for (const node of nodes) { + if (node.type === 'item') { + const strKey = String(node.key); + lm.set(strKey, node.textValue || strKey); + km.set(strKey, node.key); + } else if (node.childNodes) { + traverse(node.childNodes); } } + }; - // Fallback: return the lookup key as-is - return lookup; - }, - [localCollectionState.collection], - ); + traverse(localCollectionState.collection); + return { labelMap: lm, keyMap: km }; + }, [localCollectionState.collection]); + // O(1) key mapping via Map (replaces O(n) iteration per key) const mappedSelectedKey = useMemo(() => { - if (selectionMode !== 'single') return null; - return effectiveSelectedKey - ? findCollectionKey(effectiveSelectedKey) - : null; - }, [selectionMode, effectiveSelectedKey, findCollectionKey]); + if (selectionMode !== 'single' || effectiveSelectedKey == null) return null; + const strKey = String(effectiveSelectedKey); + return keyMap.get(strKey) ?? effectiveSelectedKey; + }, [selectionMode, effectiveSelectedKey, keyMap]); const mappedSelectedKeys = useMemo(() => { if (selectionMode !== 'multiple') return undefined; - if (effectiveSelectedKeys === 'all') return 'all' as const; - if (Array.isArray(effectiveSelectedKeys)) { - return (effectiveSelectedKeys as Key[]).map((k) => findCollectionKey(k)); + return effectiveSelectedKeys.map((k) => { + const strKey = String(k); + return keyMap.get(strKey) ?? k; + }); } - return effectiveSelectedKeys; - }, [selectionMode, effectiveSelectedKeys, findCollectionKey]); - - // Given an iterable of keys (array or Set) toggle membership for duplicates - const processSelectionArray = (iterable: Iterable): string[] => { - const resultSet = new Set(); - for (const key of iterable) { - const nKey = String(key); - if (resultSet.has(nKey)) { - resultSet.delete(nKey); // toggle off if clicked twice - } else { - resultSet.add(nKey); // select - } - } - return Array.from(resultSet); - }; - - // Helper to get selected item labels for display using local collection - const getSelectedLabels = () => { - const collection = localCollectionState.collection; - - // Helper to recursively collect all item labels from collection (including nested in sections) - const collectAllLabels = (): string[] => { - const allLabels: string[] = []; - - const traverse = (nodes: Iterable) => { - for (const node of nodes) { - if (node.type === 'item') { - allLabels.push(node.textValue || String(node.key)); - } else if (node.childNodes) { - traverse(node.childNodes); - } - } - }; + }, [selectionMode, effectiveSelectedKeys, keyMap]); - traverse(collection); - return allLabels; - }; - - // Handle "all" selection - return all available labels + // Memoized label extraction using the labelMap + const selectedLabels = useMemo(() => { if (selectionMode === 'multiple' && effectiveSelectedKeys === 'all') { - return collectAllLabels(); + return Array.from(labelMap.values()); } - const selectedSet = new Set( - selectionMode === 'multiple' && effectiveSelectedKeys !== 'all' - ? (effectiveSelectedKeys || []).map((k) => String(k)) - : effectiveSelectedKey != null - ? [String(effectiveSelectedKey)] - : [], - ); - - const labels: string[] = []; - const processedKeys = new Set(); - - // Use collection.getItem() to directly retrieve items by key (works with sections) - selectedSet.forEach((key) => { - const item = collection.getItem(key); - if (item) { - labels.push(item.textValue || String(item.key)); - processedKeys.add(String(item.key)); - } - }); - - // Handle custom values that aren't in the collection - const selectedKeysArr = + const selectedKeyStrs = selectionMode === 'multiple' && effectiveSelectedKeys !== 'all' ? (effectiveSelectedKeys || []).map(String) : effectiveSelectedKey != null ? [String(effectiveSelectedKey)] : []; - // Add labels for any selected keys that weren't found in collection (custom values) - selectedKeysArr.forEach((key) => { - if (!processedKeys.has(String(key))) { - // This is a custom value, use the key as the label - labels.push(key); - } - }); + return selectedKeyStrs.map((k) => labelMap.get(k) ?? k); + }, [selectionMode, effectiveSelectedKey, effectiveSelectedKeys, labelMap]); - return labels; - }; - - const selectedLabels = getSelectedLabels(); const hasSelection = selectedLabels.length > 0; - // Always keep the latest selection in a ref (with normalized keys) so that we can read it synchronously in the popover close effect. + // Refs for tracking selection state synchronously const latestSelectionRef = useRef<{ single: string | null; multiple: 'all' | string[]; @@ -478,45 +403,122 @@ export const FilterPicker = forwardRef(function FilterPicker( : (effectiveSelectedKeys ?? []).map(String), }; }, [effectiveSelectedKey, effectiveSelectedKeys]); + const selectionsWhenClosed = useRef<{ single: string | null; multiple: 'all' | string[]; }>({ single: null, multiple: [] }); - // Capture the initial selection (from defaultSelectedKey(s)) so that - // the very first popover open can already use it for sorting. useEffect(() => { - selectionsWhenClosed.current = { ...latestSelectionRef.current }; - }, []); // run only once on mount - - // Function to sort children with selected items on top - const getSortedChildren = useCallback(() => { - // Warn if sorting is explicitly requested but not supported - if (sortSelectedToTopExplicit && sortSelectedToTop && !items) { - console.warn( - 'FilterPicker: sortSelectedToTop only works with the items prop. ' + - 'Sorting will be skipped when using JSX children.', - ); + if (!isPopoverOpen) { + selectionsWhenClosed.current = { + single: + effectiveSelectedKey != null ? String(effectiveSelectedKey) : null, + multiple: + effectiveSelectedKeys === 'all' + ? 'all' + : (effectiveSelectedKeys ?? []).map(String), + }; } + }, [effectiveSelectedKey, effectiveSelectedKeys, isPopoverOpen]); - // Return children as-is (no sorting for JSX children) - return children; - }, [children, sortSelectedToTop, sortSelectedToTopExplicit, items]); + // --------------------------------------------------------------------------- + // Popover lifecycle — all effects moved out of the inline renderTrigger + // function so they have a stable component identity and don't tear + // down/setup on every parent re-render. + // DialogTrigger is controlled via isOpen/onOpenChange. + // --------------------------------------------------------------------------- - // Compute sorted items array when using `items` prop - const getSortedItems = useCallback(() => { - if (!items) return items; + const handleOpenChange = useEvent((isOpen: boolean) => { + if (isOpen === isPopoverOpen) return; - // Only sort if explicitly enabled - if (!sortSelectedToTop) { - return items; + if (isOpen) { + triggerWidthRef.current = + triggerRef?.current?.UNSAFE_getDOMNode()?.offsetWidth; } + setIsPopoverOpen(isOpen); + if (!isOpen) { + selectionsWhenClosed.current = { ...latestSelectionRef.current }; + cachedItemsOrder.current = null; + } + onOpenChange?.(isOpen); + }); + + // Close this picker when another menu opens (event bus) + useEffect(() => { + return on('popover:open', (data: { menuId: string }) => { + if (data.menuId !== filterPickerId && isPopoverOpen) { + handleOpenChange(false); + } + }); + }, [on, filterPickerId, isPopoverOpen, handleOpenChange]); - // Reuse the cached order if we have it. We only compute the sorted array - // once when the pop-over is opened. Cache is cleared on close. - if (cachedItemsOrder.current) { - return cachedItemsOrder.current; + // Emit event when this picker opens + useEffect(() => { + if (isPopoverOpen) { + emit('popover:open', { menuId: filterPickerId }); } + }, [isPopoverOpen, emit, filterPickerId]); + + // Position update management + const [shouldUpdatePosition, setShouldUpdatePosition] = useState(true); + + useEffect(() => { + if (isPopoverOpen) { + setShouldUpdatePosition(true); + const timerId = window.setTimeout( + () => setShouldUpdatePosition(false), + 160, + ); + return () => window.clearTimeout(timerId); + } else { + setShouldUpdatePosition(true); + } + }, [isPopoverOpen]); + + // Keyboard handler for arrow keys to open popover + const { keyboardProps } = useKeyboard({ + onKeyDown: (e) => { + if ((e.key === 'ArrowUp' || e.key === 'ArrowDown') && !isPopoverOpen) { + e.preventDefault(); + handleOpenChange(true); + } + }, + }); + + // Clear handler + const clearValue = useEvent(() => { + if (selectionMode === 'multiple') { + if (!isControlledMultiple) { + setInternalSelectedKeys([]); + } + onSelectionChange?.([]); + } else { + if (!isControlledSingle) { + setInternalSelectedKey(null); + } + onSelectionChange?.(null); + } + + handleOpenChange(false); + triggerRef?.current?.focus?.(); + props.onClear?.(); + + return false; + }); + + // --------------------------------------------------------------------------- + // Sorting + // --------------------------------------------------------------------------- + + // Ref values (selectionsWhenClosed.current) are read synchronously inside + // the memo body; isPopoverOpen changing triggers recomputation at the right + // time (the ref is updated before the next render). + const finalItems = useMemo(() => { + if (!items) return items; + if (!sortSelectedToTop) return items; + if (!isPopoverOpen) return items; + if (cachedItemsOrder.current) return cachedItemsOrder.current; const selectedSet = new Set(); @@ -526,7 +528,6 @@ export const FilterPicker = forwardRef(function FilterPicker( if (selectionMode === 'multiple') { if (selectionsWhenClosed.current.multiple === 'all') { - // Do not sort when all selected – keep original order return items; } (selectionsWhenClosed.current.multiple as string[]).forEach(addSelected); @@ -540,7 +541,6 @@ export const FilterPicker = forwardRef(function FilterPicker( return items; } - // Helpers to extract key from item object const getItemKey = (obj: unknown): string | undefined => { if (obj == null || typeof obj !== 'object') return undefined; @@ -557,7 +557,6 @@ export const FilterPicker = forwardRef(function FilterPicker( arr.forEach((obj) => { const item = obj as ItemWithKey; if (obj && Array.isArray(item.children)) { - // Section-like object – keep order, but sort its children const sortedChildren = sortArray(item.children); unselectedArr.push({ ...item, children: sortedChildren }); } else { @@ -578,28 +577,16 @@ export const FilterPicker = forwardRef(function FilterPicker( : Array.from(items as Iterable); const sorted = sortArray(itemsArray) as T[]; - if (isPopoverOpen || !cachedItemsOrder.current) { - cachedItemsOrder.current = sorted; - } + cachedItemsOrder.current = sorted; return sorted; - }, [ - items, - sortSelectedToTop, - selectionMode, - isPopoverOpen, - selectionsWhenClosed.current.multiple, - selectionsWhenClosed.current.single, - ]); - - const finalItems = getSortedItems(); + }, [items, sortSelectedToTop, selectionMode, isPopoverOpen]); - // FilterListBox handles custom values internally when allowsCustomValue={true} - // We provide sorted children (if any) and sorted items - const finalChildren = getSortedChildren(); + // --------------------------------------------------------------------------- + // Trigger content + // --------------------------------------------------------------------------- - const renderTriggerContent = () => { - // When there is a selection and a custom summary renderer is provided – use it. + const triggerContent = useMemo((): ReactNode => { if (typeof renderSummary === 'function') { if (selectionMode === 'single') { return renderSummary({ @@ -620,165 +607,155 @@ export const FilterPicker = forwardRef(function FilterPicker( return null; } - let content: ReactNode = ''; - if (!hasSelection) { return {placeholder}; } else if (selectionMode === 'single') { - content = selectedLabels[0]; + return selectedLabels[0] || null; } else if (effectiveSelectedKeys === 'all') { - content = selectAllLabel; + return selectAllLabel; } else { - content = selectedLabels.join(', '); - } - - if (!content) { - return null; + return selectedLabels.join(', ') || null; } + }, [ + renderSummary, + selectionMode, + selectedLabels, + effectiveSelectedKey, + effectiveSelectedKeys, + hasSelection, + placeholder, + selectAllLabel, + ]); - return content; - }; - - const [shouldUpdatePosition, setShouldUpdatePosition] = useState(true); - - // Capture trigger width for overlay min-width - const triggerWidth = triggerRef?.current?.UNSAFE_getDOMNode()?.offsetWidth; - - // The trigger is rendered as a function so we can access the dialog state - const renderTrigger = (state) => { - // Listen for other menus opening and close this one if needed - useEffect(() => { - const unsubscribe = on('popover:open', (data: { menuId: string }) => { - // If another menu is opening and this FilterPicker is open, close this one - if (data.menuId !== filterPickerId && state.isOpen) { - state.close(); - } - }); + const showClearButton = + isClearable && hasSelection && !isDisabled && !props.isReadOnly; + + // Trigger element — plain JSX with no hooks. + // The element type (ItemButton) is stable so React can reconcile efficiently. + const triggerElement = ( + + ) : rightIcon !== undefined ? ( + rightIcon + ) : showClearButton ? ( + } + size={size} + theme={validationState === 'invalid' ? 'danger' : undefined} + qa="FilterPickerClearButton" + mods={{ pressed: false }} + onPress={clearValue} + /> + ) : ( + + ) + } + prefix={prefix} + suffix={suffix} + hotkeys={hotkeys} + tooltip={triggerTooltip} + description={triggerDescription} + descriptionPlacement={descriptionPlacement} + styles={triggerStyles} + {...keyboardProps} + aria-label={`${props['aria-label'] ?? props.label ?? ''}`} + > + {triggerContent} + + ); - return unsubscribe; - }, [on, filterPickerId, state]); + // --------------------------------------------------------------------------- + // Selection change handler + // --------------------------------------------------------------------------- - // Emit event when this FilterPicker opens - useEffect(() => { - if (state.isOpen) { - emit('popover:open', { menuId: filterPickerId }); - } - }, [state.isOpen, emit, filterPickerId]); - - // Track popover open/close state to control sorting - useEffect(() => { - if (state.isOpen !== isPopoverOpen) { - setIsPopoverOpen(state.isOpen); - if (!state.isOpen) { - // Popover just closed – record the latest selection for the next opening - // and clear the cached order so the next session can compute afresh. - selectionsWhenClosed.current = { ...latestSelectionRef.current }; - cachedItemsOrder.current = null; - } - onOpenChange?.(state.isOpen); + const handleSelectionChange = useEvent((selection: any) => { + if (selectionMode === 'single') { + if (!isControlledSingle) { + setInternalSelectedKey(selection as Key | null); } - }, [state.isOpen, isPopoverOpen, onOpenChange]); - - // Add keyboard support for arrow keys to open the popover - const { keyboardProps } = useKeyboard({ - onKeyDown: (e) => { - if ((e.key === 'ArrowUp' || e.key === 'ArrowDown') && !state.isOpen) { - e.preventDefault(); - state.open(); + } else { + if (!isControlledMultiple) { + let normalized: 'all' | Key[] = selection; + + if (selection === 'all') { + normalized = 'all'; + } else if (Array.isArray(selection)) { + normalized = processSelectionArray(selection); + } else if ( + selection && + typeof selection === 'object' && + selection instanceof Set + ) { + normalized = processSelectionArray(selection as Set); } - }, - }); - useEffect(() => { - // Allow initial positioning & flipping when opening, then lock placement after transition - // Popover transition is ~120ms, give it a bit more time to finalize placement - if (state.isOpen) { - setShouldUpdatePosition(true); - const id = window.setTimeout(() => setShouldUpdatePosition(false), 160); - return () => window.clearTimeout(id); - } else { - setShouldUpdatePosition(true); + setInternalSelectedKeys(normalized); } - }, [state.isOpen]); - - // Clear button logic - let showClearButton = - isClearable && hasSelection && !isDisabled && !props.isReadOnly; + } - // Clear function - let clearValue = useEvent(() => { - if (selectionMode === 'multiple') { - if (!isControlledMultiple) { - setInternalSelectedKeys([]); - } - onSelectionChange?.([]); + // Update latest selection ref synchronously + if (selectionMode === 'single') { + latestSelectionRef.current.single = + selection != null ? String(selection) : null; + } else { + if (selection === 'all') { + latestSelectionRef.current.multiple = 'all'; + } else if (Array.isArray(selection)) { + latestSelectionRef.current.multiple = Array.from( + new Set(processSelectionArray(selection)), + ); + } else if ( + selection && + typeof selection === 'object' && + selection instanceof Set + ) { + latestSelectionRef.current.multiple = Array.from( + new Set(processSelectionArray(selection as Set)), + ); } else { - if (!isControlledSingle) { - setInternalSelectedKey(null); - } - onSelectionChange?.(null); - } - - if (state.isOpen) { - state.close(); + latestSelectionRef.current.multiple = + selection === 'all' + ? 'all' + : Array.isArray(selection) + ? selection.map(String) + : []; } + } - triggerRef?.current?.focus?.(); + onSelectionChange?.(selection); - props.onClear?.(); + if (selectionMode === 'single') { + handleOpenChange(false); + } + }); - return false; - }); + // Stable callbacks for popover content (avoid inline closures that change every render) + const handleEscape = useEvent(() => { + handleOpenChange(false); + }); - return ( - - ) : rightIcon !== undefined ? ( - rightIcon - ) : showClearButton ? ( - } - size={size} - theme={validationState === 'invalid' ? 'danger' : undefined} - qa="FilterPickerClearButton" - mods={{ pressed: false }} - onPress={clearValue} - /> - ) : ( - - ) - } - prefix={prefix} - suffix={suffix} - hotkeys={hotkeys} - tooltip={triggerTooltip} - description={triggerDescription} - descriptionPlacement={descriptionPlacement} - styles={triggerStyles} - {...keyboardProps} - aria-label={`${props['aria-label'] ?? props.label ?? ''}`} - > - {renderTriggerContent()} - - ); - }; + const handleOptionClick = useEvent((key: Key) => { + if ((selectionMode === 'multiple' && isCheckable) || key === '__ALL__') { + handleOpenChange(false); + } + }); const filterPickerField = ( ( {...filterBaseProps(otherProps, { eventProps: true })} > { const menuTriggerEl = el.closest('[data-popover-trigger]'); - // If no menu trigger was clicked, allow closing if (!menuTriggerEl) return true; - // If the same trigger that opened this popover was clicked, allow closing (toggle) - if (menuTriggerEl === (triggerRef as any)?.current) return true; - // Otherwise, don't close here. Let the event bus handle closing when the other opens. + if (menuTriggerEl === triggerRef?.current?.UNSAFE_getDOMNode()) + return true; return false; }} + onOpenChange={handleOpenChange} > - {renderTrigger} - {(close) => ( + {triggerElement} + {() => ( ( ...popoverStyles, }} style={ - triggerWidth - ? ({ '--overlay-min-width': `${triggerWidth}px` } as any) + triggerWidthRef.current + ? ({ + '--overlay-min-width': `${triggerWidthRef.current}px`, + } as any) : undefined } > @@ -824,7 +803,6 @@ export const FilterPicker = forwardRef(function FilterPicker( ( customValueProps={customValueProps} newCustomValueProps={newCustomValueProps} onSearchChange={onSearchChange} - onEscape={() => close()} - onOptionClick={(key) => { - // For FilterPicker, clicking the content area should close the popover - // in multiple selection mode (single mode already closes via onSelectionChange) - if ( - (selectionMode === 'multiple' && isCheckable) || - key === '__ALL__' - ) { - close(); - } - }} - onSelectionChange={(selection) => { - // No need to change any flags - children order is cached - - // Update internal state if uncontrolled - if (selectionMode === 'single') { - if (!isControlledSingle) { - setInternalSelectedKey(selection as Key | null); - } - } else { - if (!isControlledMultiple) { - let normalized: 'all' | Key[] = selection as - | 'all' - | Key[]; - - if (selection === 'all') { - normalized = 'all'; - } else if (Array.isArray(selection)) { - normalized = processSelectionArray(selection); - } else if ( - selection && - typeof selection === 'object' && - (selection as any) instanceof Set - ) { - normalized = processSelectionArray( - selection as Set, - ); - } - - setInternalSelectedKeys(normalized); - } - } - - // Update latest selection ref synchronously - if (selectionMode === 'single') { - latestSelectionRef.current.single = - selection != null ? String(selection) : null; - } else { - if (selection === 'all') { - latestSelectionRef.current.multiple = 'all'; - } else if (Array.isArray(selection)) { - latestSelectionRef.current.multiple = Array.from( - new Set(processSelectionArray(selection)), - ); - } else if ( - selection && - typeof selection === 'object' && - (selection as any) instanceof Set - ) { - latestSelectionRef.current.multiple = Array.from( - new Set(processSelectionArray(selection as Set)), - ); - } else { - latestSelectionRef.current.multiple = - selection === 'all' - ? 'all' - : Array.isArray(selection) - ? selection.map(String) - : []; - } - } - - onSelectionChange?.(selection); - - if (selectionMode === 'single') { - close(); - } - }} + onEscape={handleEscape} + onOptionClick={handleOptionClick} + onSelectionChange={handleSelectionChange} > { (children - ? (finalChildren as CollectionChildren) + ? (children as CollectionChildren) : undefined) as CollectionChildren } diff --git a/src/components/fields/Picker/Picker.tsx b/src/components/fields/Picker/Picker.tsx index b4fb1157c..4be685289 100644 --- a/src/components/fields/Picker/Picker.tsx +++ b/src/components/fields/Picker/Picker.tsx @@ -17,7 +17,6 @@ import { MutableRefObject, ReactElement, ReactNode, - useCallback, useEffect, useMemo, useRef, @@ -32,6 +31,7 @@ import { CloseIcon, DirectionIcon, LoadingIcon } from '../../../icons'; import { useProviderProps } from '../../../provider'; import { generateRandomId } from '../../../utils/random'; import { useEventBus } from '../../../utils/react/useEventBus'; +import { processSelectionArray } from '../../../utils/selection'; import { extractStyles } from '../../../utils/styles'; import { CubeItemButtonProps, ItemAction, ItemButton } from '../../actions'; import { CubeItemProps } from '../../content/Item'; @@ -267,9 +267,11 @@ export const Picker = forwardRef(function Picker( 'all' | Key[] >(defaultSelectedKeys ?? []); - // Track popover open/close and capture children order for session + // Popover state — used as controlled prop for DialogTrigger const [isPopoverOpen, setIsPopoverOpen] = useState(false); const triggerRef = useRef>(null); + // Measured lazily on popover open instead of on every render + const triggerWidthRef = useRef(undefined); const isControlledSingle = selectedKey !== undefined; const isControlledMultiple = selectedKeys !== undefined; @@ -281,20 +283,6 @@ export const Picker = forwardRef(function Picker( ? selectedKeys : internalSelectedKeys; - // Given an iterable of keys (array or Set) toggle membership for duplicates - const processSelectionArray = (iterable: Iterable): string[] => { - const resultSet = new Set(); - for (const key of iterable) { - const nKey = String(key); - if (resultSet.has(nKey)) { - resultSet.delete(nKey); // toggle off if clicked twice - } else { - resultSet.add(nKey); // select - } - } - return Array.from(resultSet); - }; - // Ref to access internal ListBox state for collection API const internalListStateRef = useRef>(null); @@ -307,22 +295,48 @@ export const Picker = forwardRef(function Picker( // Cache for sorted items array when using `items` prop const cachedItemsOrder = useRef(null); + + const latestSelectionRef = useRef<{ + single: string | null; + multiple: string[]; + }>({ + single: effectiveSelectedKey != null ? String(effectiveSelectedKey) : null, + multiple: + selectionMode === 'multiple' && effectiveSelectedKeys !== 'all' + ? (effectiveSelectedKeys || []).map(String) + : [], + }); + + useEffect(() => { + latestSelectionRef.current = { + single: + effectiveSelectedKey != null ? String(effectiveSelectedKey) : null, + multiple: + selectionMode === 'multiple' && effectiveSelectedKeys !== 'all' + ? (effectiveSelectedKeys || []).map(String) + : [], + }; + }, [effectiveSelectedKey, effectiveSelectedKeys, selectionMode]); + const selectionWhenClosed = useRef<{ single: string | null; multiple: string[]; }>({ single: null, multiple: [] }); - // Track if sortSelectedToTop was explicitly provided const sortSelectedToTopExplicit = sortSelectedToTop !== undefined; - // Default to true if items are provided, false otherwise const shouldSortSelectedToTop = sortSelectedToTop ?? (items ? true : false); - // Invalidate cache when items change + useWarn(sortSelectedToTopExplicit && shouldSortSelectedToTop && !items, { + key: ['picker-sort-selected-to-top-children'], + args: [ + 'Picker: sortSelectedToTop only works with the items prop. Sorting will be skipped when using JSX children.', + ], + }); + useEffect(() => { cachedItemsOrder.current = null; }, [items]); - // Capture selection when popover closes useEffect(() => { if (!isPopoverOpen) { selectionWhenClosed.current = { @@ -333,54 +347,36 @@ export const Picker = forwardRef(function Picker( ? (effectiveSelectedKeys || []).map(String) : [], }; - cachedItemsOrder.current = null; } }, [ - isPopoverOpen, effectiveSelectedKey, effectiveSelectedKeys, + isPopoverOpen, selectionMode, ]); - // Call onOpenChange when popover state changes - useEffect(() => { - onOpenChange?.(isPopoverOpen); - }, [isPopoverOpen]); - - // Sort items with selected on top if enabled - const getSortedItems = useCallback((): typeof items => { + const finalItems = useMemo(() => { if (!items || !shouldSortSelectedToTop) return items; + if (!isPopoverOpen) return items; + if (cachedItemsOrder.current) return cachedItemsOrder.current; - // Reuse cached order if available - if (cachedItemsOrder.current) { - return cachedItemsOrder.current; - } - - // Warn if explicitly requested but JSX children used - if (sortSelectedToTopExplicit && !items) { - console.warn( - 'Picker: sortSelectedToTop only works with the items prop. ' + - 'Sorting will be skipped when using JSX children.', - ); - return items; - } - - const selectedKeys = new Set(); + const selectedKeySet = new Set(); if (selectionMode === 'multiple') { - // Don't sort when "all" is selected if ( selectionWhenClosed.current.multiple.length === 0 || effectiveSelectedKeys === 'all' ) { return items; } - selectionWhenClosed.current.multiple.forEach((k) => selectedKeys.add(k)); + selectionWhenClosed.current.multiple.forEach((k) => + selectedKeySet.add(k), + ); } else if (selectionWhenClosed.current.single) { - selectedKeys.add(selectionWhenClosed.current.single); + selectedKeySet.add(selectionWhenClosed.current.single); } - if (selectedKeys.size === 0) return items; + if (selectedKeySet.size === 0) return items; const itemsArray = Array.isArray(items) ? items : Array.from(items); const selectedItems: T[] = []; @@ -388,7 +384,7 @@ export const Picker = forwardRef(function Picker( itemsArray.forEach((item) => { const key = (item as any)?.key ?? (item as any)?.id; - if (key != null && selectedKeys.has(String(key))) { + if (key != null && selectedKeySet.has(String(key))) { selectedItems.push(item); } else { unselectedItems.push(item); @@ -397,21 +393,10 @@ export const Picker = forwardRef(function Picker( const sorted = [...selectedItems, ...unselectedItems]; - if (isPopoverOpen) { - cachedItemsOrder.current = sorted; - } + cachedItemsOrder.current = sorted; return sorted; - }, [ - items, - shouldSortSelectedToTop, - sortSelectedToTopExplicit, - selectionMode, - effectiveSelectedKeys, - isPopoverOpen, - ]); - - const finalItems = getSortedItems(); + }, [items, shouldSortSelectedToTop, selectionMode, isPopoverOpen]); // Create local collection state for reading item data (labels, etc.) // This allows us to read item labels even before the popover opens @@ -421,28 +406,13 @@ export const Picker = forwardRef(function Picker( selectionMode: 'none', // Don't manage selection in this state }); - // Helper to get label from local collection - const getItemLabel = useCallback( - (key: Key): string => { - const item = localCollectionState?.collection?.getItem(key); - return item?.textValue || String(key); - }, - [localCollectionState?.collection], - ); - const selectedLabels = useMemo(() => { - const keysToGet = - selectionMode === 'multiple' && effectiveSelectedKeys !== 'all' - ? effectiveSelectedKeys || [] - : effectiveSelectedKey != null - ? [effectiveSelectedKey] - : []; + const collection = localCollectionState?.collection; - // Handle "all" selection if (selectionMode === 'multiple' && effectiveSelectedKeys === 'all') { - if (!localCollectionState?.collection) return []; + if (!collection) return []; const labels: string[] = []; - for (const item of localCollectionState.collection) { + for (const item of collection) { if (item.type === 'item') { labels.push(item.textValue || String(item.key)); } @@ -450,20 +420,118 @@ export const Picker = forwardRef(function Picker( return labels; } - // Get labels for selected keys - return keysToGet.map((key) => getItemLabel(key)).filter(Boolean); + const keysToGet = + selectionMode === 'multiple' && effectiveSelectedKeys !== 'all' + ? effectiveSelectedKeys || [] + : effectiveSelectedKey != null + ? [effectiveSelectedKey] + : []; + + return keysToGet + .map((key) => { + const item = collection?.getItem(key); + return item?.textValue || String(key); + }) + .filter(Boolean); }, [ selectionMode, effectiveSelectedKeys, effectiveSelectedKey, - getItemLabel, localCollectionState?.collection, ]); const hasSelection = selectedLabels.length > 0; - const renderTriggerContent = () => { - // When there is a selection and a custom summary renderer is provided – use it. + // --------------------------------------------------------------------------- + // Popover lifecycle — all effects moved out of the inline renderTrigger + // function so they have a stable component identity and don't tear + // down/setup on every parent re-render. + // DialogTrigger is controlled via isOpen/onOpenChange. + // --------------------------------------------------------------------------- + + const handleOpenChange = useEvent((isOpen: boolean) => { + if (isOpen === isPopoverOpen) return; + + if (isOpen) { + triggerWidthRef.current = + triggerRef?.current?.UNSAFE_getDOMNode()?.offsetWidth; + } + setIsPopoverOpen(isOpen); + if (!isOpen) { + selectionWhenClosed.current = { ...latestSelectionRef.current }; + cachedItemsOrder.current = null; + } + onOpenChange?.(isOpen); + }); + + // Close this picker when another menu opens (event bus) + useEffect(() => { + return on('popover:open', (data: { menuId: string }) => { + if (data.menuId !== pickerId && isPopoverOpen) { + handleOpenChange(false); + } + }); + }, [on, pickerId, isPopoverOpen, handleOpenChange]); + + // Emit event when this picker opens + useEffect(() => { + if (isPopoverOpen) { + emit('popover:open', { menuId: pickerId }); + } + }, [isPopoverOpen, emit, pickerId]); + + // Position update management + const [shouldUpdatePosition, setShouldUpdatePosition] = useState(true); + + useEffect(() => { + if (isPopoverOpen) { + setShouldUpdatePosition(true); + const timerId = window.setTimeout( + () => setShouldUpdatePosition(false), + 160, + ); + return () => window.clearTimeout(timerId); + } else { + setShouldUpdatePosition(true); + } + }, [isPopoverOpen]); + + // Keyboard handler for arrow keys to open popover + const { keyboardProps } = useKeyboard({ + onKeyDown: (e) => { + if ((e.key === 'ArrowUp' || e.key === 'ArrowDown') && !isPopoverOpen) { + e.preventDefault(); + handleOpenChange(true); + } + }, + }); + + // Clear handler + const clearValue = useEvent(() => { + if (selectionMode === 'multiple') { + if (!isControlledMultiple) { + setInternalSelectedKeys([]); + } + onSelectionChange?.([]); + } else { + if (!isControlledSingle) { + setInternalSelectedKey(null); + } + onSelectionChange?.(null); + } + + handleOpenChange(false); + triggerRef?.current?.focus?.(); + onClear?.(); + + return false; + }); + + // --------------------------------------------------------------------------- + // Trigger content + // --------------------------------------------------------------------------- + + const triggerContent = useMemo((): ReactNode => { if (typeof renderSummary === 'function') { if (selectionMode === 'single') { return renderSummary({ @@ -484,158 +552,145 @@ export const Picker = forwardRef(function Picker( return null; } - let content: ReactNode = ''; - if (!hasSelection) { return {placeholder}; } else if (selectionMode === 'single') { - content = selectedLabels[0]; + return selectedLabels[0] || null; } else if (effectiveSelectedKeys === 'all') { - content = selectAllLabel; + return selectAllLabel; } else { - content = selectedLabels.join(', '); + return selectedLabels.join(', ') || null; } + }, [ + renderSummary, + selectionMode, + selectedLabels, + effectiveSelectedKey, + effectiveSelectedKeys, + hasSelection, + placeholder, + selectAllLabel, + ]); - if (!content) { - return null; - } - - return content; - }; - - const [shouldUpdatePosition, setShouldUpdatePosition] = useState(true); - - // Capture trigger width for overlay min-width - const triggerWidth = triggerRef?.current?.UNSAFE_getDOMNode()?.offsetWidth; - - // The trigger is rendered as a function so we can access the dialog state - const renderTrigger = (state) => { - // Listen for other menus opening and close this one if needed - useEffect(() => { - const unsubscribe = on('popover:open', (data: { menuId: string }) => { - // If another menu is opening and this Picker is open, close this one - if (data.menuId !== pickerId && state.isOpen) { - state.close(); - } - }); - - return unsubscribe; - }, [on, pickerId, state]); - - // Emit event when this Picker opens - useEffect(() => { - if (state.isOpen) { - emit('popover:open', { menuId: pickerId }); + const showClearButton = + isClearable && hasSelection && !isDisabled && !props.isReadOnly; + + // Trigger element — plain JSX with no hooks. + const triggerElement = ( + + ) : rightIcon !== undefined ? ( + rightIcon + ) : showClearButton ? ( + } + size={size} + theme={validationState === 'invalid' ? 'danger' : undefined} + qa="PickerClearButton" + mods={{ pressed: false }} + onPress={clearValue} + /> + ) : ( + + ) } - }, [state.isOpen, emit, pickerId]); + prefix={prefix} + suffix={suffix} + hotkeys={hotkeys} + tooltip={triggerTooltip} + description={triggerDescription} + descriptionPlacement={descriptionPlacement} + styles={triggerStyles} + {...keyboardProps} + aria-label={`${props['aria-label'] ?? props.label ?? ''}`} + > + {triggerContent} + + ); - // Track popover open/close state to control sorting - useEffect(() => { - if (state.isOpen !== isPopoverOpen) { - setIsPopoverOpen(state.isOpen); + // --------------------------------------------------------------------------- + // Selection change handler + // --------------------------------------------------------------------------- + + const handleSelectionChange = useEvent((selection: any) => { + if (selectionMode === 'single') { + if (!isControlledSingle) { + setInternalSelectedKey(selection as Key | null); } - }, [state.isOpen, isPopoverOpen]); - - // Add keyboard support for arrow keys to open the popover - const { keyboardProps } = useKeyboard({ - onKeyDown: (e) => { - if ((e.key === 'ArrowUp' || e.key === 'ArrowDown') && !state.isOpen) { - e.preventDefault(); - state.open(); + } else { + if (!isControlledMultiple) { + let normalized: 'all' | Key[] = selection; + + if (selection === 'all') { + normalized = 'all'; + } else if (Array.isArray(selection)) { + normalized = processSelectionArray(selection); + } else if ( + selection && + typeof selection === 'object' && + selection instanceof Set + ) { + normalized = processSelectionArray(selection as Set); } - }, - }); - useEffect(() => { - // Allow initial positioning & flipping when opening, then lock placement after transition - // Popover transition is ~120ms, give it a bit more time to finalize placement - if (state.isOpen) { - setShouldUpdatePosition(true); - const id = window.setTimeout(() => setShouldUpdatePosition(false), 160); - return () => window.clearTimeout(id); - } else { - setShouldUpdatePosition(true); + setInternalSelectedKeys(normalized); } - }, [state.isOpen]); - - // Clear button logic - let showClearButton = - isClearable && hasSelection && !isDisabled && !props.isReadOnly; + } - // Clear function - let clearValue = useEvent(() => { - if (selectionMode === 'multiple') { - if (!isControlledMultiple) { - setInternalSelectedKeys([]); - } - onSelectionChange?.([]); + if (selectionMode === 'single') { + latestSelectionRef.current.single = + selection != null ? String(selection) : null; + } else { + if (selection === 'all') { + latestSelectionRef.current.multiple = []; + } else if (Array.isArray(selection)) { + latestSelectionRef.current.multiple = processSelectionArray(selection); + } else if ( + selection && + typeof selection === 'object' && + selection instanceof Set + ) { + latestSelectionRef.current.multiple = processSelectionArray( + selection as Set, + ); } else { - if (!isControlledSingle) { - setInternalSelectedKey(null); - } - onSelectionChange?.(null); - } - - if (state.isOpen) { - state.close(); + latestSelectionRef.current.multiple = []; } + } - triggerRef?.current?.focus?.(); + onSelectionChange?.(selection); - onClear?.(); + if (selectionMode === 'single') { + handleOpenChange(false); + } + }); - return false; - }); + const handleEscape = useEvent(() => { + handleOpenChange(false); + }); - return ( - - ) : rightIcon !== undefined ? ( - rightIcon - ) : showClearButton ? ( - } - size={size} - theme={validationState === 'invalid' ? 'danger' : undefined} - qa="PickerClearButton" - mods={{ pressed: false }} - onPress={clearValue} - /> - ) : ( - - ) - } - prefix={prefix} - suffix={suffix} - hotkeys={hotkeys} - tooltip={triggerTooltip} - description={triggerDescription} - descriptionPlacement={descriptionPlacement} - styles={triggerStyles} - {...keyboardProps} - aria-label={`${props['aria-label'] ?? props.label ?? ''}`} - > - {renderTriggerContent()} - - ); - }; + const handleOptionClick = useEvent((key: Key) => { + if ((selectionMode === 'multiple' && isCheckable) || key === '__ALL__') { + handleOpenChange(false); + } + }); const pickerField = ( ( {...filterBaseProps(otherProps, { eventProps: true })} > { const menuTriggerEl = el.closest('[data-popover-trigger]'); - // If no menu trigger was clicked, allow closing if (!menuTriggerEl) return true; - // If the same trigger that opened this popover was clicked, allow closing (toggle) - if (menuTriggerEl === (triggerRef as any)?.current) return true; - // Otherwise, don't close here. Let the event bus handle closing when the other opens. + if (menuTriggerEl === triggerRef?.current?.UNSAFE_getDOMNode()) + return true; return false; }} + onOpenChange={handleOpenChange} > - {renderTrigger} - {(close) => ( + {triggerElement} + {() => ( ( ...popoverStyles, }} style={ - triggerWidth - ? ({ '--overlay-min-width': `${triggerWidth}px` } as any) + triggerWidthRef.current + ? ({ + '--overlay-min-width': `${triggerWidthRef.current}px`, + } as any) : undefined } > @@ -680,7 +737,6 @@ export const Picker = forwardRef(function Picker( ( footerStyles={footerStyles} qa={`${props.qa || 'Picker'}ListBox`} allValueProps={allValueProps} - onEscape={() => close()} - onOptionClick={(key) => { - // For Picker, clicking the content area should close the popover - // in multiple selection mode (single mode already closes via onSelectionChange) - if ( - (selectionMode === 'multiple' && isCheckable) || - key === '__ALL__' - ) { - close(); - } - }} - onSelectionChange={(selection) => { - // No need to change any flags - children order is cached - - // Update internal state if uncontrolled - if (selectionMode === 'single') { - if (!isControlledSingle) { - setInternalSelectedKey(selection as Key | null); - } - } else { - if (!isControlledMultiple) { - let normalized: 'all' | Key[] = selection as - | 'all' - | Key[]; - - if (selection === 'all') { - normalized = 'all'; - } else if (Array.isArray(selection)) { - normalized = processSelectionArray(selection); - } else if ( - selection && - typeof selection === 'object' && - (selection as any) instanceof Set - ) { - normalized = processSelectionArray( - selection as Set, - ); - } - - setInternalSelectedKeys(normalized); - } - } - - onSelectionChange?.(selection); - - if (selectionMode === 'single') { - close(); - } - }} + onEscape={handleEscape} + onOptionClick={handleOptionClick} + onSelectionChange={handleSelectionChange} > {children as CollectionChildren} diff --git a/src/utils/index.ts b/src/utils/index.ts index f499004f3..0a009d55e 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -27,3 +27,6 @@ export { range } from './range'; // ResizeSensor export { ResizeSensor } from './ResizeSensor'; + +// Selection utilities +export { processSelectionArray } from './selection'; diff --git a/src/utils/selection.ts b/src/utils/selection.ts new file mode 100644 index 000000000..0ce9da4de --- /dev/null +++ b/src/utils/selection.ts @@ -0,0 +1,18 @@ +import { Key } from 'react-aria'; + +/** + * Deduplicate/toggle keys in a selection: if a key appears twice it is + * removed (toggled off), otherwise it is kept (selected). + */ +export function processSelectionArray(iterable: Iterable): string[] { + const resultSet = new Set(); + for (const key of iterable) { + const nKey = String(key); + if (resultSet.has(nKey)) { + resultSet.delete(nKey); + } else { + resultSet.add(nKey); + } + } + return Array.from(resultSet); +}