Fixes for cmd+k search palette#846
Conversation
👷 Deploy request for tanstack pending review.Visit the deploys page to approve it
|
📝 WalkthroughWalkthroughThe SearchModal's interaction and focus behavior are refined: modifier key presses during Hit activation now return early to prevent navigation, explicit focus scheduling via ChangesSearch Modal Interaction & Focus Refinement
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/SearchModal.tsx (1)
424-429: Consider also guarding non-primary mouse buttons.The modifier-key check covers cmd/ctrl/shift/alt + click, but middle-click (
event.button === 1) also opens links in a new tab on most browsers and will currently close the palette. If you want the "open multiple results" flow to be consistent across input methods, include a button check:- if (event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) { + if ( + event.button !== 0 || + event.metaKey || + event.ctrlKey || + event.shiftKey || + event.altKey + ) { return }Note: many browsers now dispatch
auxclickinstead ofclickfor middle-button, so this is defensive rather than strictly necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SearchModal.tsx` around lines 424 - 429, The click handler handleClick currently only checks modifier keys and should also ignore non-primary mouse buttons so middle-clicks don't close the search; update handleClick to short-circuit when event.button !== 0 (or explicitly check for event.button === 1) before calling closeSearch, and add or wire an auxclick handler mirroring the same guard (so middle-button/Aux clicks are consistently ignored). Refer to handleClick, closeSearch and the auxclick event when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/SearchModal.tsx`:
- Around line 785-800: The effect that focuses the search input schedules two
nested requestAnimationFrame calls but only cancels the outer id; update the
effect (useEffect in SearchModal.tsx) to capture both the outer and inner
requestAnimationFrame ids (e.g., idOuter and idInner) when scheduling
focusSearchInput (which uses containerRef and
querySelector('input[type="search"]')) and cancel both in the cleanup via
window.cancelAnimationFrame(idOuter) and window.cancelAnimationFrame(idInner) so
the inner callback cannot run after the component closes or unmounts.
---
Nitpick comments:
In `@src/components/SearchModal.tsx`:
- Around line 424-429: The click handler handleClick currently only checks
modifier keys and should also ignore non-primary mouse buttons so middle-clicks
don't close the search; update handleClick to short-circuit when event.button
!== 0 (or explicitly check for event.button === 1) before calling closeSearch,
and add or wire an auxclick handler mirroring the same guard (so
middle-button/Aux clicks are consistently ignored). Refer to handleClick,
closeSearch and the auxclick event when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cebd3aa-985d-475a-a6a3-41b0ab319ba7
📒 Files selected for processing (1)
src/components/SearchModal.tsx
| React.useEffect(() => { | ||
| if (!isOpen) return | ||
|
|
||
| const focusSearchInput = () => { | ||
| const input = containerRef.current?.querySelector<HTMLInputElement>( | ||
| 'input[type="search"]', | ||
| ) | ||
| input?.focus() | ||
| } | ||
|
|
||
| const id = window.requestAnimationFrame(() => { | ||
| window.requestAnimationFrame(focusSearchInput) | ||
| }) | ||
| return () => window.cancelAnimationFrame(id) | ||
| }, [isOpen]) | ||
|
|
There was a problem hiding this comment.
Cleanup only cancels the outer requestAnimationFrame.
If the outer rAF has already fired before the effect cleanup runs (e.g., user closes the palette within ~1 frame of opening, or isOpen toggles quickly), the inner rAF is already scheduled and will still invoke focusSearchInput() — potentially focusing an input that's about to unmount or stealing focus after close. Track the inner id and cancel it as well.
♻️ Proposed fix
- const id = window.requestAnimationFrame(() => {
- window.requestAnimationFrame(focusSearchInput)
- })
- return () => window.cancelAnimationFrame(id)
+ let innerId = 0
+ const outerId = window.requestAnimationFrame(() => {
+ innerId = window.requestAnimationFrame(focusSearchInput)
+ })
+ return () => {
+ window.cancelAnimationFrame(outerId)
+ if (innerId) window.cancelAnimationFrame(innerId)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/SearchModal.tsx` around lines 785 - 800, The effect that
focuses the search input schedules two nested requestAnimationFrame calls but
only cancels the outer id; update the effect (useEffect in SearchModal.tsx) to
capture both the outer and inner requestAnimationFrame ids (e.g., idOuter and
idInner) when scheduling focusSearchInput (which uses containerRef and
querySelector('input[type="search"]')) and cancel both in the cleanup via
window.cancelAnimationFrame(idOuter) and window.cancelAnimationFrame(idInner) so
the inner callback cannot run after the component closes or unmounts.
|
YES! i was also wondering why search input was not auto focus when the search panel is opened! |
|
This should fix #848 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/SearchModal.tsx (1)
448-479:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the actual click event into
handleActivate.Line 461 references an undefined
eventvariable. SincehandleActivate()has no parameters andhandleClick()doesn't capture or forward the click event, accessingevent.metaKey,event.ctrlKey, etc. will throw a ReferenceError at runtime. The modifier-key guard will fail completely when triggered.♻️ Suggested fix
- const handleActivate = () => { + const handleActivate = (event: React.MouseEvent<HTMLAnchorElement>) => { const framework = hit.framework if ( framework && shouldPersistFrameworkForHit({ url: hit.url, @@ if (event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) { return } closeSearch() } @@ - const handleClick = () => { - handleActivate() - } - const ref = React.useRef<HTMLAnchorElement>(null!) @@ - onClick={handleClick} + onClick={handleActivate}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SearchModal.tsx` around lines 448 - 479, handleActivate currently reads an undefined global `event`; update it to accept an event parameter (e.g. handleActivate = (event?: MouseEvent | KeyboardEvent) => { ... }) and replace usages of `event.metaKey`/`event.ctrlKey`/`event.shiftKey`/`event.altKey` with checks on that parameter (e.g. if (event?.metaKey || ... ) return). Then change handleClick to capture and forward the click event (e.g. const handleClick = (e: React.MouseEvent) => handleActivate(e)). Ensure existing keyboard code that calls handleActivate without an event still works by keeping the parameter optional and using optional chaining in the modifier checks.
♻️ Duplicate comments (1)
src/components/SearchModal.tsx (1)
832-846:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCancel the inner
requestAnimationFrametoo.If cleanup runs after the outer frame fires, the callback queued on Line 843 still survives and can refocus the search input after the dialog closes or unmounts.
♻️ Suggested fix
React.useEffect(() => { if (!isOpen) return const focusSearchInput = () => { const input = containerRef.current?.querySelector<HTMLInputElement>( 'input[type="search"]', ) input?.focus() } - const id = window.requestAnimationFrame(() => { - window.requestAnimationFrame(focusSearchInput) - }) - return () => window.cancelAnimationFrame(id) + let innerId = 0 + const outerId = window.requestAnimationFrame(() => { + innerId = window.requestAnimationFrame(focusSearchInput) + }) + return () => { + window.cancelAnimationFrame(outerId) + if (innerId) window.cancelAnimationFrame(innerId) + } }, [isOpen])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SearchModal.tsx` around lines 832 - 846, The effect sets two nested requestAnimationFrame calls but only cancels the outer id; modify the useEffect so you capture both ids (e.g., outerId and innerId) when scheduling window.requestAnimationFrame and window.requestAnimationFrame(focusSearchInput) and cancel both in the cleanup using window.cancelAnimationFrame(outerId) and window.cancelAnimationFrame(innerId); reference the existing focusSearchInput, containerRef, and isOpen symbols to locate where to store and cancel the inner animation frame so the input won't be focused after the modal closes or unmounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/SearchModal.tsx`:
- Around line 448-479: handleActivate currently reads an undefined global
`event`; update it to accept an event parameter (e.g. handleActivate = (event?:
MouseEvent | KeyboardEvent) => { ... }) and replace usages of
`event.metaKey`/`event.ctrlKey`/`event.shiftKey`/`event.altKey` with checks on
that parameter (e.g. if (event?.metaKey || ... ) return). Then change
handleClick to capture and forward the click event (e.g. const handleClick = (e:
React.MouseEvent) => handleActivate(e)). Ensure existing keyboard code that
calls handleActivate without an event still works by keeping the parameter
optional and using optional chaining in the modifier checks.
---
Duplicate comments:
In `@src/components/SearchModal.tsx`:
- Around line 832-846: The effect sets two nested requestAnimationFrame calls
but only cancels the outer id; modify the useEffect so you capture both ids
(e.g., outerId and innerId) when scheduling window.requestAnimationFrame and
window.requestAnimationFrame(focusSearchInput) and cancel both in the cleanup
using window.cancelAnimationFrame(outerId) and
window.cancelAnimationFrame(innerId); reference the existing focusSearchInput,
containerRef, and isOpen symbols to locate where to store and cancel the inner
animation frame so the input won't be focused after the modal closes or
unmounts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e19d4d25-cc12-4b86-b980-58315a6ac95b
📒 Files selected for processing (1)
src/components/SearchModal.tsx
3 participants
Summary by CodeRabbit
Release Notes