Skip to content

fix(cmdk): clean up search modal input handling #3147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
waleedlatif1 wants to merge 2 commits into
base: staging
Choose a base branch
from
+28 −34
Open

fix(cmdk): clean up search modal input handling #3147

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1cfa125
fix(cmdk): clean up search modal input handling
waleedlatif1 Feb 5, 2026
f83d9ee
ack comments
waleedlatif1 Feb 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 28 additions & 34 deletions ...app/workspace/[workspaceId]/w/components/sidebar/components/search-modal/search-modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ export function SearchModal({
const router = useRouter()
const workspaceId = params.workspaceId as string
const inputRef = useRef<HTMLInputElement>(null)
const listRef = useRef<HTMLDivElement>(null)
const [mounted, setMounted] = useState(false)
const [search, setSearch] = useState('')
const openSettingsModal = useSettingsModalStore((state) => state.openModal)
const { config: permissionConfig } = usePermissionConfig()

Expand Down Expand Up @@ -142,41 +144,18 @@ export function SearchModal({
)

useEffect(() => {
if (open && inputRef.current) {
const nativeInputValueSetter = Object.getOwnPropertyDescriptor(
window.HTMLInputElement.prototype,
'value'
)?.set
if (nativeInputValueSetter) {
nativeInputValueSetter.call(inputRef.current, '')
inputRef.current.dispatchEvent(new Event('input', { bubbles: true }))
}
inputRef.current.focus()
if (open) {
setSearch('')
inputRef.current?.focus()
}
}, [open])

const handleSearchChange = useCallback(() => {
requestAnimationFrame(() => {
const list = document.querySelector('[cmdk-list]')
if (list) {
list.scrollTop = 0
}
})
}, [])

useEffect(() => {
if (!open) return

const handleKeyDown = (e: KeyboardEvent) => {
if (e.key === 'Escape') {
e.preventDefault()
onOpenChange(false)
}
const handleSearchChange = useCallback((value: string) => {
setSearch(value)
if (listRef.current) {
listRef.current.scrollTop = 0
}

document.addEventListener('keydown', handleKeyDown)
return () => document.removeEventListener('keydown', handleKeyDown)
}, [open, onOpenChange])
}, [])
Copy link

@cursor cursor bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web


const handleBlockSelect = useCallback(
(block: SearchBlockItem, type: 'block' | 'trigger' | 'tool') => {
Expand Down Expand Up @@ -264,7 +243,7 @@ export function SearchModal({
{/* Overlay */}
<div
className={cn(
'fixed inset-0 z-40 bg-[#E4E4E4]/50 backdrop-blur-[0.75px] transition-opacity duration-100 dark:bg-[#0D0D0D]/50',
'fixed inset-0 z-40 bg-[#E4E4E4]/50 transition-opacity duration-100 dark:bg-[#0D0D0D]/50',
open ? 'opacity-100' : 'pointer-events-none opacity-0'
)}
onClick={() => onOpenChange(false)}
Expand All @@ -281,16 +260,31 @@ export function SearchModal({
'-translate-x-1/2 fixed top-[15%] left-1/2 z-50 w-[500px] overflow-hidden rounded-[12px] border border-[var(--border)] bg-[var(--surface-4)] shadow-lg',
open ? 'visible opacity-100' : 'invisible opacity-0'
)}
onKeyDown={(e) => {
if (e.key === 'Escape') {
e.preventDefault()
onOpenChange(false)
}
}}
>
<Command label='Search' filter={customFilter}>
<Command.Input
ref={inputRef}
autoFocus
value={search}
onValueChange={handleSearchChange}
onKeyDown={(e) => {
if (e.key === 'Escape') {
e.preventDefault()
onOpenChange(false)
}
}}
Copy link

@cursor cursor bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Fix in Cursor Fix in Web

placeholder='Search anything...'
className='w-full border-0 border-[var(--border)] border-b bg-transparent px-[12px] py-[10px] font-base text-[15px] text-[var(--text-primary)] placeholder:text-[var(--text-secondary)] focus:outline-none'
/>
<Command.List className='scrollbar-thin scrollbar-thumb-border scrollbar-track-transparent max-h-[400px] overflow-y-auto p-[8px]'>
<Command.List
ref={listRef}
className='scrollbar-thin scrollbar-thumb-border scrollbar-track-transparent max-h-[400px] overflow-y-auto p-[8px]'
>
<Command.Empty className='flex items-center justify-center px-[16px] py-[24px] text-[15px] text-[var(--text-subtle)]'>
No results found.
</Command.Empty>
Expand Down