fix(cmdk): clean up search modal input handling#3147
fix(cmdk): clean up search modal input handling#3147waleedlatif1 wants to merge 2 commits intostagingfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile OverviewGreptile SummaryThis PR refactors the workspace sidebar SearchModal (cmdk) to remove a native input value setter hack, replace a querySelector+RAF scroll reset with a ref, and scope Escape-to-close to a component Main issue to address before merge: the new Escape handling is attached to a non-focusable dialog wrapper div, which won’t reliably receive key events. The prior implementation listened on Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant SM as SearchModal
participant I as Input(ref)
participant L as List(ref)
U->>SM: Open modal
SM->>I: Render controlled value
U->>I: Type query
I-->>SM: onValueChange(query)
SM->>SM: setSearchQuery(query)
SM->>L: scrollTop = 0 via ref
U->>SM: Press Escape
SM-->>SM: onKeyDown(Escape)
SM-->>U: Close modal
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/search-modal/search-modal.tsx
Line: 254:268
Comment:
**Escape key no longer works**
`onKeyDown` is attached to the outer `<div role="dialog">`, but that element isn’t focusable and won’t reliably receive keyboard events. Previously, Escape was handled via a `document` keydown listener when `open` was true, so Escape would close the modal regardless of focus. With the current change, pressing Escape while focus is inside `Command.Input`/cmdk internals may not bubble to this `div`, leaving the modal stuck open for keyboard users. Consider handling Escape on the focused element (e.g., `Command` root / `Command.Input`) or make the dialog focusable and ensure it’s focused on open.
How can I resolve this? If you propose a fix, please make it concise. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
...im/app/workspace/[workspaceId]/w/components/sidebar/components/search-modal/search-modal.tsx
Show resolved
Hide resolved
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| document.addEventListener('keydown', handleKeyDown) | ||
| return () => document.removeEventListener('keydown', handleKeyDown) | ||
| }, [open, onOpenChange]) | ||
| }, []) |
There was a problem hiding this comment.
Synchronous scroll reset occurs before DOM update
Low Severity
In handleSearchChange, listRef.current.scrollTop = 0 runs synchronously before React commits the re-render triggered by setSearch(value). The old code intentionally used requestAnimationFrame to delay the scroll reset until after the DOM updated with filtered results. Because cmdk internally calls scrollIntoView on the newly selected item after re-rendering, the synchronous reset gets overridden, making it effectively a no-op in cases where the list needs scrolling back to the top.
| e.preventDefault() | ||
| onOpenChange(false) | ||
| } | ||
| }} |
There was a problem hiding this comment.
Duplicate Escape key handlers cause redundant calls
Low Severity
The onKeyDown Escape handler on Command.Input is redundant with the identical handler on the parent dialog div. Since the input is a child of the div, the keydown event bubbles up naturally, so the parent handler already covers all Escape presses from within the dialog. Having both means onOpenChange(false) gets called twice per Escape press when the input is focused.
Additional Locations (1)
1 participant
Summary
Type of Change
Testing
Tested manually
Checklist