shadcn-ui / ui

Beautifully designed components that you can copy and paste into your apps. Accessible. Customizable. Open Source.
https://ui.shadcn.com
MIT License
75.68k stars 4.75k forks source link

[bug]: Controlled combobox search #4264

Open BrendanC23 opened 4 months ago

BrendanC23 commented 4 months ago

Describe the bug

I have a combobox (which uses cmdk 1.0.0, though this issue happens with older versions as well). When the search input is controlled, like this:

<CommandInput
    value={search}
    onValueChange={setSearch}
/>

and the combobox is closed and the search input is cleared after an input is selected

<CommandItem
    setOpen(false);
    setSearch("");
    }}
>

the full list is rendered for a moment, before the combobox is closed.

Affected component/components

Combobox

How to reproduce

  1. Type "Next.js" into the search bar
  2. Click on the displayed option
  3. Notice that all of the options are rendered for a moment before the combobox is closed.

If the controlled search props are removed, this does not happen.

value={search}
onValueChange={setSearch}

Here's a video showing the issue:

https://github.com/user-attachments/assets/03aacd60-d7b7-4107-bd9a-d5d1c12e2571

Codesandbox/StackBlitz link

https://codesandbox.io/p/devbox/shadcn-ui-vite-forked-p26mvj

Logs

No response

System Info

Codesandbox

Before submitting

ahmedsemih commented 4 months ago

I think this is not a bug. In the video, "Next.js" is already selected. If you click on it again, this will deselect it. If you click on another option, it will select that option instead of clearing the input. This behavior is caused by the onSelect prop of the CommandItem.

onSelect={(currentValue: string) => {
    setValue(currentValue === value ? "" : currentValue);  //  This line makes it work like I said.
    setOpen(false);
    setSearch("");
 }}

If you want the selected option not to be deselected when you click on it, you can change the code to something like this:

onSelect={(currentValue: string) => {
    setValue(currentValue); 
    setOpen(false);
    setSearch("");
 }}
BrendanC23 commented 4 months ago

The issue isn't that Next.js is deselected. As you said, the code is written specifically so that this happens.

The issue occurs when value={search} is used. When a user types into the search field, a filtered list of options are displayed. Once one of them is chosen (either selected for the first time or de-delrcted), the combobox closed. This is intentional and caused by setOpen(false). Before the combobox closes, however, there is a brief moment where the full list of options are rendered.

This also happens if you start with no selection, search for Next.js, and then select it. The list of all of the items is shown for a moment before the combobox is closed.

The current flow is like this:

  1. User searches for a value, is presented with a filtered list of options, and selects one
  2. A re-render happens and the search input is cleared, the full list is shown (instead of just the filtered values), and Next.js is de-selected
  3. Another re-render happens and the combobox is closed

The second item above should not happen. Once an item is clicked, the combobox should close.

ahmedsemih commented 4 months ago

Okay, I got it now. I apologize for the misunderstanding.

thought7878 commented 4 months ago

The issue isn't that Next.js is deselected. As you said, the code is written specifically so that this happens.

The issue occurs when value={search} is used. When a user types into the search field, a filtered list of options are displayed. Once one of them is chosen (either selected for the first time or de-delrcted), the combobox closed. This is intentional and caused by setOpen(false). Before the combobox closes, however, there is a brief moment where the full list of options are rendered.使用 value={search} 时会出现此问题。当用户在搜索字段中键入内容时,会显示经过筛选的选项列表。一旦选择其中一个(无论是第一次选择还是取消删除),组合框就会关闭。这是故意的,是由 setOpen(false) 引起的。然而,在组合框关闭之前,会短暂呈现完整的选项列表。

This also happens if you start with no selection, search for Next.js, and then select it. The list of all of the items is shown for a moment before the combobox is closed.

The current flow is like this:

  1. User searches for a value, is presented with a filtered list of options, and selects one
  2. A re-render happens and the search input is cleared, the full list is shown (instead of just the filtered values), and Next.js is de-selected
  3. Another re-render happens and the combobox is closed

The second item above should not happen. Once an item is clicked, the combobox should close.

If the search state is removed, this bug is gone. I think the search state is not necessary.

<CommandInput
  placeholder='Search framework...'
  // CHANGE ME
  // value={search}
  // onValueChange={setSearch}
/>
kulterryan commented 3 weeks ago

Try adding shouldFilter={false} in <Command> should resolve the issue.

Ref