fix: CommandDialog not closing on ESC #608
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (all 1 issue)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/ui/src/components/command.tsx">
<violation number="1" location="packages/ui/src/components/command.tsx:160">
P2: The conditional `{...(dialogOnOpenChange ? { open } : {})}` ignores the user-provided `open` prop when `Command` is used standalone (outside `CommandDialog`). This changes the default behavior since `open={true}` was previously always passed. Consider always passing the `open` prop to preserve backward compatibility.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| keepHighlight={keepHighlight} | ||
| open={open} | ||
| onOpenChange={shouldHandleEscape ? handleOpenChange : undefined} | ||
| {...(dialogOnOpenChange ? { open } : {})} |
There was a problem hiding this comment.
P2: The conditional {...(dialogOnOpenChange ? { open } : {})} ignores the user-provided open prop when Command is used standalone (outside CommandDialog). This changes the default behavior since open={true} was previously always passed. Consider always passing the open prop to preserve backward compatibility.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ui/src/components/command.tsx, line 160:
<comment>The conditional `{...(dialogOnOpenChange ? { open } : {})}` ignores the user-provided `open` prop when `Command` is used standalone (outside `CommandDialog`). This changes the default behavior since `open={true}` was previously always passed. Consider always passing the `open` prop to preserve backward compatibility.</comment>
<file context>
@@ -100,13 +128,36 @@ function Command({
keepHighlight={keepHighlight}
- open={open}
+ onOpenChange={shouldHandleEscape ? handleOpenChange : undefined}
+ {...(dialogOnOpenChange ? { open } : {})}
{...props}
/>
</file context>
| {...(dialogOnOpenChange ? { open } : {})} | |
| open={open} |
✅ Addressed in 9a12843
pasqualevitiello
changed the title
| onOpenChange, | ||
| ...props | ||
| }: CommandDialogPrimitive.Root.Props) { | ||
| const handleClose = React.useCallback( |
There was a problem hiding this comment.
Is handleClose necessary? Is there a need to override the eventDetails parameter?
| keepHighlight={keepHighlight} | ||
| open={open} | ||
| onOpenChange={shouldHandleEscape ? handleOpenChange : undefined} | ||
| {...(dialogOnOpenChange ? { open } : {})} |
There was a problem hiding this comment.
I find this easier to read.
| {...(dialogOnOpenChange ? { open } : {})} | |
| open={dialogOnOpenChange ? open : undefined} |
| className, | ||
| )} | ||
| data-slot="command-dialog-popup" | ||
| initialFocus={inputRef} |
There was a problem hiding this comment.
Not directly related to this PR but, is this ref really needed? Without it, the command input is still focused when the dialog opens. Is it to prevent the focus going anywhere else if the user includes other interactive element before the input inside the dialog? Could autoFocus work in that case? autoFocus would be problematic when using Command standalone without a dialog, right?
There was a problem hiding this comment.
@aarongarciah I introduced it to make the input autofocus on touch devices, because it's not working by default. Do you think I should use autoFocus instead?
There was a problem hiding this comment.
tbh I don't think we'd need a standalone Command
| onOpenChange, | ||
| ...props | ||
| }: React.ComponentProps<typeof Autocomplete>) { | ||
| const { dialogOnOpenChange } = React.useContext(CommandContext); |
There was a problem hiding this comment.
There's an alternative solution to avoid this piece of context. It's a bit of a hack but simplifies the implementation. The idea is to use a hidden Dialog close button and click it programatically. It's hacky because afaik the Command component can be used standalone without a dialog wrapping it. Just leaving this as an alternative.
const closeRef = React.useRef<HTMLButtonElement>(null);
const handleOpenChange = React.useCallback<
NonNullable<typeof onOpenChange>
>(
(nextOpen, eventDetails) => {
if (eventDetails.reason === "escape-key") {
closeRef.current?.click();
}
onOpenChange?.(nextOpen, eventDetails);
},
[onOpenChange],
);
// ...
<>
<Autocomplete
...
/>
{/* Hidden close button for programmatic closing on Escape */}
<CommandDialogPrimitive.Close ref={closeRef} hidden />
</>| {...props} | ||
| > | ||
| <CommandInputContext.Provider value={{ inputRef }}> | ||
| <CommandContext.Provider value={{ ...context, inputRef }}> |
There was a problem hiding this comment.
I'm not used to see contexts being used like this i.e. nest them to later append something to the value. Seems like there could be two different contexts? One for the dialog and another one for the input. Is there a reason to join them under the same context?
There was a problem hiding this comment.
@aarongarciah Not sure why I overcomplicated it so much :D Now I have a much cleaner component which seems to be working properly on both touch and desktop. I think this component should be designed to be always used inside a dialog, and not as "standalone" Command. That said, I still have a concern: with this implementation, the CommandDialog should always be used as a controlled component, otherwise you can't close the dialog on item click. Do you think it should work like that?
There was a problem hiding this comment.
Much nicer, yeah! You could use the hidden Dialog.Close hack to achieve an uncontrolled component. AFAIK it's the only way of making it uncontrolled since there's no access to Base UI components' internal store/context currently.
There was a problem hiding this comment.
Precious piece of advice, thank you!
…commanddialog-not-closing-on-esc
Summary by cubic
Fixes CommandDialog not closing when pressing Esc and updates the footer hint to show Esc, aligning with CUI-100. Also closes the palette when navigating to a page.
Bug Fixes
Refactors
Written for commit 04ac29e. Summary will update automatically on new commits.
Fixes: #607