From 19789cf5f8a1236d5592db301c893afa7644a3a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laura=20Fern=C3=A1ndez?= Date: Mon, 24 Feb 2025 13:12:56 +0100 Subject: [PATCH] Combobox: use `useOptions` (#100604) --- .betterer.results | 3 - .../components/MetricCombobox.test.tsx | 25 +-- .../src/components/Combobox/Combobox.test.tsx | 19 +- .../src/components/Combobox/Combobox.tsx | 165 +++++------------- .../src/components/Combobox/useOptions.ts | 21 +-- 5 files changed, 77 insertions(+), 156 deletions(-) diff --git a/.betterer.results b/.betterer.results index b4f67dde103..eedd45faab7 100644 --- a/.betterer.results +++ b/.betterer.results @@ -540,9 +540,6 @@ exports[`better eslint`] = { [0, 0, 0, "No untranslated strings in text props. Wrap text with or use t()", "0"], [0, 0, 0, "No untranslated strings in text props. Wrap text with or use t()", "1"] ], - "packages/grafana-ui/src/components/Combobox/Combobox.tsx:5381": [ - [0, 0, 0, "Do not use any type assertions.", "0"] - ], "packages/grafana-ui/src/components/Combobox/MultiCombobox.tsx:5381": [ [0, 0, 0, "Do not use any type assertions.", "0"] ], diff --git a/packages/grafana-prometheus/src/querybuilder/components/MetricCombobox.test.tsx b/packages/grafana-prometheus/src/querybuilder/components/MetricCombobox.test.tsx index c9ff1d55cd2..c832a547bdc 100644 --- a/packages/grafana-prometheus/src/querybuilder/components/MetricCombobox.test.tsx +++ b/packages/grafana-prometheus/src/querybuilder/components/MetricCombobox.test.tsx @@ -35,9 +35,13 @@ describe('MetricCombobox', () => { } as unknown as DataSourceInstanceSettings; const mockDatasource = new PrometheusDatasource(instanceSettings); - const mockValues = [{ label: 'random_metric' }, { label: 'unique_metric' }, { label: 'more_unique_metric' }]; - // Mock metricFindQuery which will call backend API + // Options returned when user first opens the combobox - returned by onGetMetrics + const initialMockValues = [{ label: 'top_metric_one' }, { label: 'top_metric_two' }, { label: 'top_metric_three' }]; + const mockOnGetMetrics = jest.fn(() => Promise.resolve(initialMockValues.map((v) => ({ value: v.label })))); + + // Options returned when user searches for a metric + const mockValues = [{ label: 'random_metric' }, { label: 'unique_metric' }, { label: 'more_unique_metric' }]; mockDatasource.metricFindQuery = jest.fn((query: string) => { // return Promise.resolve([]); // Use the label values regex to get the values inside the label_values function call @@ -61,7 +65,6 @@ describe('MetricCombobox', () => { }); const mockOnChange = jest.fn(); - const mockOnGetMetrics = jest.fn(() => Promise.resolve(mockValues.map((v) => ({ value: v.label })))); const defaultProps: MetricComboboxProps = { metricLookupDisabled: false, @@ -92,10 +95,11 @@ describe('MetricCombobox', () => { const combobox = screen.getByPlaceholderText('Select metric'); await userEvent.click(combobox); - expect(mockOnGetMetrics).toHaveBeenCalledTimes(1); - - const item = await screen.findByRole('option', { name: 'random_metric' }); + const item = await screen.findByRole('option', { name: 'top_metric_one' }); expect(item).toBeInTheDocument(); + + // This should be asserted by the above check, but double check anyway + expect(mockOnGetMetrics).toHaveBeenCalledTimes(1); }); it('fetches metrics for the users query', async () => { @@ -108,8 +112,9 @@ describe('MetricCombobox', () => { const item = await screen.findByRole('option', { name: 'unique_metric' }); expect(item).toBeInTheDocument(); - const negativeItem = screen.queryByRole('option', { name: 'random_metric' }); - expect(negativeItem).not.toBeInTheDocument(); + // This should be asserted by the above check, but double check anyway + // This is the actual argument, created by formatKeyValueStringsForLabelValuesQuery() + expect(mockDatasource.metricFindQuery).toHaveBeenCalledWith('label_values({__name__=~".*unique.*"},__name__)'); }); it('calls onChange with the correct value when a metric is selected', async () => { @@ -118,10 +123,10 @@ describe('MetricCombobox', () => { const combobox = screen.getByPlaceholderText('Select metric'); await userEvent.click(combobox); - const item = await screen.findByRole('option', { name: 'random_metric' }); + const item = await screen.findByRole('option', { name: 'top_metric_two' }); await userEvent.click(item); - expect(mockOnChange).toHaveBeenCalledWith({ metric: 'random_metric', labels: [], operations: [] }); + expect(mockOnChange).toHaveBeenCalledWith({ metric: 'top_metric_two', labels: [], operations: [] }); }); it('shows the metrics explorer button by default', () => { diff --git a/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx b/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx index 6127c007e56..e54c94d1977 100644 --- a/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx +++ b/packages/grafana-ui/src/components/Combobox/Combobox.test.tsx @@ -1,4 +1,4 @@ -import { act, render, screen, fireEvent } from '@testing-library/react'; +import { act, render, screen, fireEvent, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React from 'react'; @@ -395,7 +395,9 @@ describe('Combobox', () => { const input = screen.getByRole('combobox'); await user.click(input); - expect(asyncSpy).toHaveBeenCalledTimes(1); // Called on open + expect(asyncSpy).not.toHaveBeenCalledTimes(1); // Not called yet + act(() => jest.advanceTimersByTime(200)); // Add the debounce time + expect(asyncSpy).toHaveBeenCalledTimes(1); // Then check if called on open asyncSpy.mockClear(); await user.keyboard('a'); @@ -434,9 +436,9 @@ describe('Combobox', () => { }); it('should display message when there is an error loading async options', async () => { - const asyncOptions = jest.fn(() => { - throw new Error('Could not retrieve options'); - }); + const fetchData = jest.fn(); + const asyncOptions = fetchData.mockRejectedValue(new Error('Could not retrieve options')); + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); render(); @@ -445,12 +447,15 @@ describe('Combobox', () => { await user.type(input, 'test'); await act(async () => { - jest.advanceTimersToNextTimer(); + jest.advanceTimersByTimeAsync(500); }); + expect(asyncOptions).rejects.toThrow('Could not retrieve options'); + await waitFor(() => expect(consoleErrorSpy).toHaveBeenCalled()); const emptyMessage = screen.queryByText('An error occurred while loading options.'); - expect(emptyMessage).toBeInTheDocument(); + + asyncOptions.mockClear(); }); describe('with a value already selected', () => { diff --git a/packages/grafana-ui/src/components/Combobox/Combobox.tsx b/packages/grafana-ui/src/components/Combobox/Combobox.tsx index f69d15b3038..2a1ea6ae28c 100644 --- a/packages/grafana-ui/src/components/Combobox/Combobox.tsx +++ b/packages/grafana-ui/src/components/Combobox/Combobox.tsx @@ -1,11 +1,9 @@ import { cx } from '@emotion/css'; import { useVirtualizer } from '@tanstack/react-virtual'; import { useCombobox } from 'downshift'; -import { debounce } from 'lodash'; -import { useCallback, useId, useMemo, useState } from 'react'; +import { useId, useMemo } from 'react'; import { useStyles2 } from '../../themes'; -import { logOptions } from '../../utils'; import { t } from '../../utils/i18n'; import { Icon } from '../Icon/Icon'; import { AutoSizeInput } from '../Input/AutoSizeInput'; @@ -14,11 +12,11 @@ import { Portal } from '../Portal/Portal'; import { ScrollContainer } from '../ScrollContainer/ScrollContainer'; import { AsyncError, NotFoundError } from './MessageRows'; -import { fuzzyFind, itemToString } from './filter'; +import { itemToString } from './filter'; import { getComboboxStyles, MENU_OPTION_HEIGHT, MENU_OPTION_HEIGHT_DESCRIPTION } from './getComboboxStyles'; import { ComboboxOption } from './types'; import { useComboboxFloat } from './useComboboxFloat'; -import { StaleResultError, useLatestAsyncCall } from './useLatestAsyncCall'; +import { useOptions } from './useOptions'; // TODO: It would be great if ComboboxOption["label"] was more generic so that if consumers do pass it in (for async), // then the onChange handler emits ComboboxOption with the label as non-undefined. @@ -64,8 +62,6 @@ export interface ComboboxBaseProps onBlur?: () => void; } -const RECOMMENDED_ITEMS_AMOUNT = 100_000; - type ClearableConditionals = | { /** @@ -102,7 +98,6 @@ export type ComboboxProps = ComboboxBaseProps & ClearableConditionals; const noop = () => {}; -const asyncNoop = () => Promise.resolve([]); export const VIRTUAL_OVERSCAN_ITEMS = 4; @@ -113,7 +108,7 @@ export const VIRTUAL_OVERSCAN_ITEMS = 4; */ export const Combobox = (props: ComboboxProps) => { const { - options, + options: allOptions, onChange, value: valueProp, placeholder: placeholderProp, @@ -135,45 +130,13 @@ export const Combobox = (props: ComboboxProps) => // get a consistent Value from it const value = typeof valueProp === 'object' ? valueProp?.value : valueProp; - const isAsync = typeof options === 'function'; - const loadOptions = useLatestAsyncCall(isAsync ? options : asyncNoop); // loadOptions isn't called at all if not async - const [asyncLoading, setAsyncLoading] = useState(false); - const [asyncError, setAsyncError] = useState(false); - - // A custom setter to always prepend the custom value at the beginning, if needed - const [items, baseSetItems] = useState(isAsync ? [] : options); - const setItems = useCallback( - (items: Array>, inputValue: string | undefined) => { - let itemsToSet = items; - logOptions(itemsToSet.length, RECOMMENDED_ITEMS_AMOUNT, id, ariaLabelledBy); - if (inputValue && createCustomValue) { - //Since the label of a normal option does not have to match its value and a custom option has the same value and label, - //we just focus on the value to check if the option already exists - const optionMatchingInput = items.find((opt) => opt.value === inputValue); - - if (!optionMatchingInput) { - const customValueOption = { - label: inputValue, - // Type casting needed to make this work when T is a number - value: inputValue as T, - description: t('combobox.custom-value.description', 'Use custom value'), - }; - - itemsToSet = items.slice(0); - itemsToSet.unshift(customValueOption); - } - } - - baseSetItems(itemsToSet); - }, - [createCustomValue, id, ariaLabelledBy] - ); - - // Memoize for using in fuzzy search - const stringifiedItems = useMemo( - () => (isAsync ? [] : options.map((item) => itemToString(item))), - [options, isAsync] - ); + const { + options: filteredOptions, + updateOptions, + asyncLoading, + asyncError, + } = useOptions(props.options, createCustomValue); + const isAsync = typeof allOptions === 'function'; const selectedItemIndex = useMemo(() => { if (isAsync) { @@ -184,13 +147,13 @@ export const Combobox = (props: ComboboxProps) => return null; } - const index = options.findIndex((option) => option.value === value); + const index = allOptions.findIndex((option) => option.value === value); if (index === -1) { return null; } return index; - }, [valueProp, options, value, isAsync]); + }, [valueProp, allOptions, value, isAsync]); const selectedItem = useMemo(() => { if (valueProp === undefined || valueProp === null) { @@ -198,11 +161,11 @@ export const Combobox = (props: ComboboxProps) => } if (selectedItemIndex !== null && !isAsync) { - return options[selectedItemIndex]; + return allOptions[selectedItemIndex]; } return typeof valueProp === 'object' ? valueProp : { value: valueProp, label: valueProp.toString() }; - }, [selectedItemIndex, isAsync, valueProp, options]); + }, [selectedItemIndex, isAsync, valueProp, allOptions]); const menuId = `downshift-${useId().replace(/:/g, '--')}-menu`; const labelId = `downshift-${useId().replace(/:/g, '--')}-label`; @@ -210,33 +173,15 @@ export const Combobox = (props: ComboboxProps) => const styles = useStyles2(getComboboxStyles); const virtualizerOptions = { - count: items.length, + count: filteredOptions.length, getScrollElement: () => scrollRef.current, - estimateSize: (index: number) => (items[index].description ? MENU_OPTION_HEIGHT_DESCRIPTION : MENU_OPTION_HEIGHT), + estimateSize: (index: number) => + filteredOptions[index].description ? MENU_OPTION_HEIGHT_DESCRIPTION : MENU_OPTION_HEIGHT, overscan: VIRTUAL_OVERSCAN_ITEMS, }; const rowVirtualizer = useVirtualizer(virtualizerOptions); - const debounceAsync = useMemo( - () => - debounce((inputValue: string) => { - loadOptions(inputValue) - .then((opts) => { - setItems(opts, inputValue); - setAsyncLoading(false); - setAsyncError(false); - }) - .catch((err) => { - if (!(err instanceof StaleResultError)) { - setAsyncError(true); - setAsyncLoading(false); - } - }); - }, 200), - [loadOptions, setItems] - ); - const { isOpen, highlightedIndex, @@ -250,7 +195,7 @@ export const Combobox = (props: ComboboxProps) => menuId, labelId, inputId: id, - items, + items: filteredOptions, itemToString, selectedItem, @@ -267,48 +212,9 @@ export const Combobox = (props: ComboboxProps) => scrollIntoView: () => {}, - onInputValueChange: ({ inputValue, isOpen }) => { - if (!isOpen) { - // Prevent stale options from showing on reopen - if (isAsync) { - setItems([], ''); - } - - // Otherwise there's nothing else to do when the menu isnt open - return; - } - - if (!isAsync) { - const filteredItems = fuzzyFind(options, stringifiedItems, inputValue); - setItems(filteredItems, inputValue); - } else { - if (inputValue && createCustomValue) { - setItems([], inputValue); - } - - setAsyncLoading(true); - debounceAsync(inputValue); - } - }, - onIsOpenChange: ({ isOpen, inputValue }) => { - // Loading async options mostly happens in onInputValueChange, but if the menu is opened with an empty input - // then onInputValueChange isn't called (because the input value hasn't changed) - if (isAsync && isOpen && inputValue === '') { - setAsyncLoading(true); - // TODO: dedupe this loading logic with debounceAsync - loadOptions(inputValue) - .then((opts) => { - setItems(opts, inputValue); - setAsyncLoading(false); - setAsyncError(false); - }) - .catch((err) => { - if (!(err instanceof StaleResultError)) { - setAsyncError(true); - setAsyncLoading(false); - } - }); + if (isOpen && inputValue === '') { + updateOptions(inputValue); } }, @@ -317,7 +223,16 @@ export const Combobox = (props: ComboboxProps) => rowVirtualizer.scrollToIndex(highlightedIndex); } }, + onStateChange: ({ inputValue: newInputValue, type, selectedItem: newSelectedItem }) => { + switch (type) { + case useCombobox.stateChangeTypes.InputChange: + updateOptions(newInputValue ?? ''); + break; + default: + break; + } + }, stateReducer(state, actionAndChanges) { let { changes } = actionAndChanges; const menuBeingOpened = state.isOpen === false && changes.isOpen === true; @@ -353,7 +268,7 @@ export const Combobox = (props: ComboboxProps) => }, }); - const { inputRef, floatingRef, floatStyles, scrollRef } = useComboboxFloat(items, isOpen); + const { inputRef, floatingRef, floatStyles, scrollRef } = useComboboxFloat(filteredOptions, isOpen); const isAutoSize = width === 'auto'; @@ -429,14 +344,16 @@ export const Combobox = (props: ComboboxProps) => {!asyncError && (
    {rowVirtualizer.getVirtualItems().map((virtualRow) => { + const item = filteredOptions[virtualRow.index]; + return (
  • (props: ComboboxProps) => transform: `translateY(${virtualRow.start}px)`, }} {...getItemProps({ - item: items[virtualRow.index], + item: item, index: virtualRow.index, })} >
    - - {items[virtualRow.index].label ?? items[virtualRow.index].value} - - {items[virtualRow.index].description && ( - {items[virtualRow.index].description} - )} + {item.label ?? item.value} + {item.description && {item.description}}
    • ); @@ -463,7 +376,7 @@ export const Combobox = (props: ComboboxProps) => )}
    {asyncError && } - {items.length === 0 && !asyncError && } + {filteredOptions.length === 0 && !asyncError && }
    )} diff --git a/packages/grafana-ui/src/components/Combobox/useOptions.ts b/packages/grafana-ui/src/components/Combobox/useOptions.ts index fc2747cee1e..fffad1613cb 100644 --- a/packages/grafana-ui/src/components/Combobox/useOptions.ts +++ b/packages/grafana-ui/src/components/Combobox/useOptions.ts @@ -3,7 +3,7 @@ import { useState, useCallback, useMemo } from 'react'; import { t } from '../../utils/i18n'; -import { itemFilter } from './filter'; +import { fuzzyFind, itemToString } from './filter'; import { ComboboxOption } from './types'; import { StaleResultError, useLatestAsyncCall } from './useLatestAsyncCall'; @@ -83,14 +83,11 @@ export function useOptions(rawOptions: AsyncOptions { - if (!isAsync) { - setUserTypedSearch(inputValue); - return; + setUserTypedSearch(inputValue); + if (isAsync) { + setAsyncLoading(true); + debouncedLoadOptions(inputValue); } - - setAsyncLoading(true); - - debouncedLoadOptions(inputValue); }, [debouncedLoadOptions, isAsync] ); @@ -122,12 +119,16 @@ export function useOptions(rawOptions: AsyncOptions { + return isAsync ? [] : rawOptions.map(itemToString); + }, [isAsync, rawOptions]); + const finalOptions = useMemo(() => { - const currentOptions = isAsync ? asyncOptions : rawOptions.filter(itemFilter(userTypedSearch)); + const currentOptions = isAsync ? asyncOptions : fuzzyFind(rawOptions, stringifiedOptions, userTypedSearch); const currentOptionsOrganised = organizeOptionsByGroup(currentOptions); return addCustomValue(currentOptionsOrganised); - }, [isAsync, organizeOptionsByGroup, addCustomValue, asyncOptions, rawOptions, userTypedSearch]); + }, [isAsync, organizeOptionsByGroup, addCustomValue, asyncOptions, rawOptions, userTypedSearch, stringifiedOptions]); return { options: finalOptions, updateOptions, asyncLoading, asyncError }; }