Combobox: Improve async UX (#96054)

* Refactor basic usage with stateReducer

This is a combination of 3 commits.
This is the 1st commit message:
more wip

This is the commit message #2:
even more wip

This is the commit message #3:
got basic usage working well with stateReducer

remove unrelated change

todo tests

* fix behaviour for async

* clean up dev stuff

* story

* Fix options being cleared for non-async combobox

* Fill out tests!

* put story back

* clean up metriccombobox test

* show selected value as placeholder while menu is open

* properly fallback placeholder to the prop

packages/grafana-prometheus/src/querybuilder/components/MetricCombobox.test.tsx

@@ -104,14 +104,12 @@ describe('MetricCombobox', () => {
 
     const combobox = screen.getByPlaceholderText('Select metric');
     await userEvent.click(combobox);
-
     await userEvent.type(combobox, 'unique');
-    expect(jest.mocked(mockDatasource.metricFindQuery)).toHaveBeenCalled();
 
     const item = await screen.findByRole('option', { name: 'unique_metric' });
     expect(item).toBeInTheDocument();
 
-    const negativeItem = await screen.queryByRole('option', { name: 'random_metric' });
+    const negativeItem = screen.queryByRole('option', { name: 'random_metric' });
     expect(negativeItem).not.toBeInTheDocument();
   });
 

packages/grafana-ui/src/components/Combobox/Combobox.story.tsx

@@ -8,7 +8,7 @@ import { useTheme2 } from '../../themes/ThemeContext';
 import { Alert } from '../Alert/Alert';
 import { Divider } from '../Divider/Divider';
 import { Field } from '../Forms/Field';
-import { Select, AsyncSelect } from '../Select/Select';
+import { AsyncSelect, Select } from '../Select/Select';
 
 import { Combobox, ComboboxOption } from './Combobox';
 
@@ -55,6 +55,7 @@ const meta: Meta<PropsAndCustomArgs> = {
 
 const BasicWithState: StoryFn<typeof Combobox> = (args) => {
   const [value, setValue] = useState(args.value);
+
   return (
     <Field label="Test input" description="Input with a few options">
       <Combobox
@@ -259,6 +260,7 @@ export const CustomValue: StoryObj<PropsAndCustomArgs> = {
   },
 };
 
+const loadOptionsAction = action('loadOptions called');
 const AsyncStory: StoryFn<PropsAndCustomArgs> = (args) => {
   // Combobox
   const [selectedOption, setSelectedOption] = useState<ComboboxOption<string> | null>(null);
@@ -268,7 +270,7 @@ const AsyncStory: StoryFn<PropsAndCustomArgs> = (args) => {
 
   // This simulates a kind of search API call
   const loadOptionsWithLabels = useCallback((inputValue: string) => {
-    console.info(`Load options called with value '${inputValue}' `);
+    loadOptionsAction(inputValue);
     return fakeSearchAPI(`http://example.com/search?query=${inputValue}`);
   }, []);
 
@@ -326,7 +328,6 @@ const AsyncStory: StoryFn<PropsAndCustomArgs> = (args) => {
 
       <Field label="Async with error" description="An odd number of characters throws an error">
         <Combobox
-          {...args}
           id="test-combobox-error"
           placeholder="Select an option"
           options={loadOptionsWithErrors}
@@ -337,6 +338,7 @@ const AsyncStory: StoryFn<PropsAndCustomArgs> = (args) => {
           }}
         />
       </Field>
+
       <Field label="Compared to AsyncSelect">
         <AsyncSelect
           id="test-async-select"
@@ -350,6 +352,20 @@ const AsyncStory: StoryFn<PropsAndCustomArgs> = (args) => {
           }}
         />
       </Field>
+
+      <Field label="Async with error" description="An odd number of characters throws an error">
+        <Combobox
+          {...args}
+          id="test-combobox-error"
+          placeholder="Select an option"
+          options={loadOptionsWithErrors}
+          value={selectedOption}
+          onChange={(val) => {
+            action('onChange')(val);
+            setSelectedOption(val);
+          }}
+        />
+      </Field>
     </>
   );
 };

packages/grafana-ui/src/components/Combobox/Combobox.test.tsx

@@ -28,6 +28,10 @@ describe('Combobox', () => {
     });
   });
 
+  afterEach(() => {
+    onChangeHandler.mockReset();
+  });
+
   it('renders without error', () => {
     render(<Combobox options={options} value={null} onChange={onChangeHandler} />);
     expect(screen.getByRole('combobox')).toBeInTheDocument();
@@ -45,6 +49,15 @@ describe('Combobox', () => {
     expect(onChangeHandler).toHaveBeenCalledWith(options[0]);
   });
 
+  it("shows the placeholder with the menu open when there's no value", async () => {
+    render(<Combobox options={options} value={null} onChange={onChangeHandler} placeholder="Select an option" />);
+
+    const input = screen.getByRole('combobox');
+    await userEvent.click(input);
+
+    expect(input).toHaveAttribute('placeholder', 'Select an option');
+  });
+
   it('selects value by clicking that needs scrolling', async () => {
     render(<Combobox options={options} value={null} onChange={onChangeHandler} />);
 
@@ -93,6 +106,54 @@ describe('Combobox', () => {
     expect(screen.queryByDisplayValue('Option 2')).not.toBeInTheDocument();
   });
 
+  describe('with a value already selected', () => {
+    it('shows an empty text input when opening the menu', async () => {
+      const selectedValue = options[0].value;
+      render(<Combobox options={options} value={selectedValue} onChange={onChangeHandler} />);
+      expect(screen.getByRole('combobox')).toBeInTheDocument();
+
+      const input = screen.getByRole('combobox');
+      await userEvent.click(input);
+
+      expect(input).toHaveValue('');
+    });
+
+    it('shows all options unfiltered when opening the menu', async () => {
+      const selectedValue = options[0].value;
+      render(<Combobox options={options} value={selectedValue} onChange={onChangeHandler} />);
+      expect(screen.getByRole('combobox')).toBeInTheDocument();
+
+      const input = screen.getByRole('combobox');
+      await userEvent.click(input);
+
+      const optionsEls = await screen.findAllByRole('option');
+      expect(optionsEls).toHaveLength(options.length);
+    });
+
+    it('shows the current selected value as the placeholder of the input', async () => {
+      const selectedValue = options[0].value;
+      render(<Combobox options={options} value={selectedValue} onChange={onChangeHandler} />);
+      expect(screen.getByRole('combobox')).toBeInTheDocument();
+
+      const input = screen.getByRole('combobox');
+      await userEvent.click(input);
+
+      expect(input).toHaveAttribute('placeholder', options[0].label);
+    });
+
+    it('exiting the menu without selecting an item restores the value to the text input', async () => {
+      const selectedValue = options[0].value;
+      render(<Combobox options={options} value={selectedValue} onChange={onChangeHandler} />);
+
+      const input = screen.getByRole('combobox');
+      await userEvent.type(input, 'Option 3');
+      await userEvent.keyboard('{Esc}');
+
+      expect(onChangeHandler).not.toHaveBeenCalled();
+      expect(input).toHaveValue('Option 1');
+    });
+  });
+
   describe('create custom value', () => {
     it('should allow creating a custom value', async () => {
       const onChangeHandler = jest.fn();
@@ -156,6 +217,8 @@ describe('Combobox', () => {
       const input = screen.getByRole('combobox');
       await user.click(input);
 
+      await act(async () => jest.advanceTimersByTime(200));
+
       expect(asyncOptions).toHaveBeenCalled();
     });
 
@@ -294,5 +357,57 @@ describe('Combobox', () => {
 
       expect(emptyMessage).toBeInTheDocument();
     });
+
+    describe('with a value already selected', () => {
+      const selectedValue = { value: '1', label: 'Option 1' };
+
+      it('shows an empty text input when opening the menu', async () => {
+        const asyncOptions = jest.fn(() => Promise.resolve(simpleAsyncOptions));
+        render(<Combobox options={asyncOptions} value={selectedValue} onChange={onChangeHandler} />);
+
+        const input = screen.getByRole('combobox');
+        await user.click(input);
+
+        // Flush out async on open changes
+        await act(async () => Promise.resolve());
+
+        expect(input).toHaveValue('');
+      });
+
+      it('shows all options unfiltered when opening the menu', async () => {
+        const asyncOptions = jest.fn(() => Promise.resolve(simpleAsyncOptions));
+        render(<Combobox options={asyncOptions} value={selectedValue} onChange={onChangeHandler} />);
+
+        const input = screen.getByRole('combobox');
+        await user.click(input);
+
+        // Flush out async on open changes
+        await act(async () => Promise.resolve());
+
+        const optionsEls = await screen.findAllByRole('option');
+        expect(optionsEls).toHaveLength(simpleAsyncOptions.length);
+      });
+
+      it('exiting the menu without selecting an item restores the value to the text input', async () => {
+        const asyncOptions = jest.fn(async () => {
+          return new Promise<ComboboxOption[]>((resolve) => setTimeout(() => resolve([{ value: 'first' }]), 2000));
+        });
+
+        render(<Combobox options={asyncOptions} value={selectedValue} onChange={onChangeHandler} createCustomValue />);
+
+        const input = screen.getByRole('combobox');
+        await user.click(input);
+
+        await act(async () => {
+          await user.type(input, 'Opt');
+          jest.advanceTimersByTime(500); // Custom value while typing
+        });
+
+        await user.keyboard('{Esc}');
+
+        expect(onChangeHandler).not.toHaveBeenCalled();
+        expect(input).toHaveValue('Option 1');
+      });
+    });
   });
 });

packages/grafana-ui/src/components/Combobox/Combobox.tsx

@@ -72,6 +72,7 @@ function itemFilter<T extends string | number>(inputValue: string) {
   };
 }
 
+const noop = () => {};
 const asyncNoop = () => Promise.resolve([]);
 
 /**
@@ -79,17 +80,20 @@ const asyncNoop = () => Promise.resolve([]);
  *
  * @alpha
  */
-export const Combobox = <T extends string | number>({
-  options,
-  onChange,
-  value: valueProp,
-  isClearable = false,
-  createCustomValue = false,
-  id,
-  width,
-  'aria-labelledby': ariaLabelledBy,
-  ...restProps
-}: ComboboxProps<T>) => {
+export const Combobox = <T extends string | number>(props: ComboboxProps<T>) => {
+  const {
+    options,
+    onChange,
+    value: valueProp,
+    placeholder: placeholderProp,
+    isClearable = false,
+    createCustomValue = false,
+    id,
+    width,
+    'aria-labelledby': ariaLabelledBy,
+    ...restProps
+  } = props;
+
   // Value can be an actual scalar Value (string or number), or an Option (value + label), so
   // get a consistent Value from it
   const value = typeof valueProp === 'object' ? valueProp?.value : valueProp;
@@ -99,7 +103,31 @@ export const Combobox = <T extends string | number>({
   const [asyncLoading, setAsyncLoading] = useState(false);
   const [asyncError, setAsyncError] = useState(false);
 
-  const [items, setItems] = useState(isAsync ? [] : options);
+  // 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<ComboboxOption<T>>, inputValue: string | undefined) => {
+      let itemsToSet = items;
+
+      if (inputValue && createCustomValue) {
+        const optionMatchingInput = items.find((opt) => opt.label === inputValue || opt.value === inputValue);
+
+        if (!optionMatchingInput) {
+          const customValueOption = {
+            // Type casting needed to make this work when T is a number
+            value: inputValue as unknown as T,
+            description: t('combobox.custom-value.create', 'Create custom value'),
+          };
+
+          itemsToSet = items.slice(0);
+          itemsToSet.unshift(customValueOption);
+        }
+      }
+
+      baseSetItems(itemsToSet);
+    },
+    [createCustomValue]
+  );
 
   const selectedItemIndex = useMemo(() => {
     if (isAsync) {
@@ -142,10 +170,10 @@ export const Combobox = <T extends string | number>({
 
   const debounceAsync = useMemo(
     () =>
-      debounce((inputValue: string, customValueOption: ComboboxOption<T> | null) => {
+      debounce((inputValue: string) => {
         loadOptions(inputValue)
           .then((opts) => {
-            setItems(customValueOption ? [customValueOption, ...opts] : opts);
+            setItems(opts, inputValue);
             setAsyncLoading(false);
             setAsyncError(false);
           })
@@ -156,16 +184,17 @@ export const Combobox = <T extends string | number>({
             }
           });
       }, 200),
-    [loadOptions]
+    [loadOptions, setItems]
   );
 
   const {
+    isOpen,
+    highlightedIndex,
+
     getInputProps,
     getMenuProps,
     getItemProps,
-    isOpen,
-    highlightedIndex,
-    setInputValue,
+
     openMenu,
     closeMenu,
     selectItem,
@@ -176,51 +205,53 @@ export const Combobox = <T extends string | number>({
     items,
     itemToString,
     selectedItem,
+
+    // Don't change downshift state in the onBlahChange handlers. Instead, use the stateReducer to make changes.
+    // Downshift calls change handlers on the render after so you can get sync/flickering issues if you change its state
+    // in them.
+    // Instead, stateReducer is called in the same tick as state changes, before that state is committed and rendered.
+
     onSelectedItemChange: ({ selectedItem }) => {
       onChange(selectedItem);
     },
+
     defaultHighlightedIndex: selectedItemIndex ?? 0,
 
     scrollIntoView: () => {},
-    onInputValueChange: ({ inputValue }) => {
-      const customValueOption =
-        createCustomValue &&
-        inputValue &&
-        items.findIndex((opt) => opt.label === inputValue || opt.value === inputValue) === -1
-          ? {
-              // Type casting needed to make this work when T is a number
-              value: inputValue as unknown as T,
-              description: t('combobox.custom-value.create', 'Create custom value'),
-            }
-          : null;
 
-      if (isAsync) {
-        if (customValueOption) {
-          setItems([customValueOption]);
+    onInputValueChange: ({ inputValue, isOpen }) => {
+      if (!isOpen) {
+        // Prevent stale options from showing on reopen
+        if (isAsync) {
+          setItems([], '');
         }
-        setAsyncLoading(true);
-        debounceAsync(inputValue, customValueOption);
 
+        // Otherwise there's nothing else to do when the menu isnt open
         return;
       }
 
-      const filteredItems = options.filter(itemFilter(inputValue));
+      if (!isAsync) {
+        const filteredItems = options.filter(itemFilter(inputValue));
+        setItems(filteredItems, inputValue);
+      } else {
+        if (inputValue && createCustomValue) {
+          setItems([], inputValue);
+        }
 
-      setItems(customValueOption ? [customValueOption, ...filteredItems] : filteredItems);
+        setAsyncLoading(true);
+        debounceAsync(inputValue);
+      }
     },
 
     onIsOpenChange: ({ isOpen, inputValue }) => {
-      // Default to displaying all values when opening
-      if (isOpen && !isAsync) {
-        setItems(options);
-        return;
-      }
-
-      if (isOpen && isAsync) {
+      // 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);
-        loadOptions(inputValue ?? '')
-          .then((options) => {
-            setItems(options);
+        // TODO: dedupe this loading logic with debounceAsync
+        loadOptions(inputValue)
+          .then((opts) => {
+            setItems(opts, inputValue);
             setAsyncLoading(false);
             setAsyncError(false);
           })
@@ -230,22 +261,52 @@ export const Combobox = <T extends string | number>({
               setAsyncLoading(false);
             }
           });
-        return;
       }
     },
+
     onHighlightedIndexChange: ({ highlightedIndex, type }) => {
       if (type !== useCombobox.stateChangeTypes.MenuMouseLeave) {
         rowVirtualizer.scrollToIndex(highlightedIndex);
       }
     },
+
+    stateReducer(state, actionAndChanges) {
+      let { changes } = actionAndChanges;
+      const menuBeingOpened = state.isOpen === false && changes.isOpen === true;
+      const menuBeingClosed = state.isOpen === true && changes.isOpen === false;
+
+      // Reset the input value when the menu is opened. If the menu is opened due to an input change
+      // then make sure we keep that.
+      // This will trigger onInputValueChange to load async options
+      if (menuBeingOpened && changes.inputValue === state.inputValue) {
+        changes = {
+          ...changes,
+          inputValue: '',
+        };
+      }
+
+      if (menuBeingClosed) {
+        // Flush the selected item to the input when the menu is closed
+        if (changes.selectedItem) {
+          changes = {
+            ...changes,
+            inputValue: itemToString(changes.selectedItem),
+          };
+        } else if (changes.inputValue !== '') {
+          // Otherwise if no selected value, clear any search from the input
+          changes = {
+            ...changes,
+            inputValue: '',
+          };
+        }
+      }
+
+      return changes;
+    },
   });
 
   const { inputRef, floatingRef, floatStyles, scrollRef } = useComboboxFloat(items, rowVirtualizer.range, isOpen);
 
-  const onBlur = useCallback(() => {
-    setInputValue(selectedItem?.label ?? value?.toString() ?? '');
-  }, [selectedItem, setInputValue, value]);
-
   const handleSuffixClick = useCallback(() => {
     isOpen ? closeMenu() : openMenu();
   }, [isOpen, openMenu, closeMenu]);
@@ -259,6 +320,8 @@ export const Combobox = <T extends string | number>({
       ? 'search'
       : 'angle-down';
 
+  const placeholder = (isOpen ? itemToString(selectedItem) : null) || placeholderProp;
+
   return (
     <div>
       <InputComponent
@@ -297,9 +360,9 @@ export const Combobox = <T extends string | number>({
            *  See issue here: https://github.com/downshift-js/downshift/issues/718
            *  Downshift repo: https://github.com/downshift-js/downshift/tree/master
            */
-          onChange: () => {},
-          onBlur,
+          onChange: noop,
           'aria-labelledby': ariaLabelledBy, // Label should be handled with the Field component
+          placeholder,
         })}
       />
       <div