fix(search): focus cmdk input on first modal open#1021
Open
whizzkid1452 wants to merge 2 commits into
Open
Conversation
The search modal looked for input[type=search], but the cmdk search field renders as a text input with cmdk-input. Focus now targets the correct input on open, preloads the modal chunk during idle time, and adds regression tests for the selector helper.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a shared search-input focus helper, rewires SearchModal and AiDock to use it, prevents Radix Dialog auto-focus, preloads the lazy search modal module on mount, and adds tests covering selector priority and focus behavior. ChangesSearch Focus Refactor and Preload
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant RadixDialog as Radix Dialog
participant SearchModal
participant scheduleSearchInputFocus
participant focusSearchInputInContainer
RadixDialog->>SearchModal: onOpenAutoFocus
SearchModal->>SearchModal: preventDefault()
SearchModal->>scheduleSearchInputFocus: contentRef.current
scheduleSearchInputFocus->>scheduleSearchInputFocus: nested requestAnimationFrame
scheduleSearchInputFocus->>focusSearchInputInContainer: container
focusSearchInputInContainer->>focusSearchInputInContainer: focus({ preventScroll: true })
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. 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 (2)
tests/search-focus.test.ts (2)
1-91: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing edge-case coverage: null container and no-match fallthrough.
There's no test for
getSearchInputFromContainer(null)(the early-return branch ingetSearchInputFromContainer) or for the case where none of the four selectors match. Both are cheap to add given the existingcreateMockContainerhelper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/search-focus.test.ts` around lines 1 - 91, Add edge-case tests in search-focus.test.ts for getSearchInputFromContainer’s early return when passed null and for the no-match fallthrough when createMockContainer exposes none of the selectors. Reuse the existing createMockContainer and createMockInput helpers, and assert the null case returns null and the no-match container also returns null. Keep the new coverage alongside the current getSearchInputFromContainer and focusSearchInputInContainer assertions so the selector behavior is fully exercised.
47-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrecedence tests don't isolate each selector; last fallback (
type="search") is never independently verified.In
cmdkContainer(Lines 47-52), both[cmdk-input]andinput[aria-label="Search TanStack"]map to the samecmdkInputobject, so the test can't tell whether the first or second selector actually matched. Similarly, insearchTypeContainer(Lines 61-66), bothinput[aria-label="Search"]andinput[type="search"]map to the samesearchTypeInput, so thetype="search"fallback path is never exercised on its own.♻️ Suggested refactor to isolate each selector
-const cmdkContainer = createMockContainer({ - '[cmdk-input]': cmdkInput, - 'input[aria-label="Search TanStack"]': cmdkInput, - 'input[aria-label="Search"]': null, - 'input[type="search"]': createMockInput({ type: 'search' }), -}) +const cmdkContainer = createMockContainer({ + '[cmdk-input]': cmdkInput, + 'input[aria-label="Search TanStack"]': null, + 'input[aria-label="Search"]': null, + 'input[type="search"]': null, +}) ... -const searchTypeContainer = createMockContainer({ - '[cmdk-input]': null, - 'input[aria-label="Search TanStack"]': null, - 'input[aria-label="Search"]': searchTypeInput, - 'input[type="search"]': searchTypeInput, -}) +const searchTypeContainer = createMockContainer({ + '[cmdk-input]': null, + 'input[aria-label="Search TanStack"]': null, + 'input[aria-label="Search"]': null, + 'input[type="search"]': searchTypeInput, +})Consider adding a dedicated case for
input[aria-label="Search TanStack"]andinput[aria-label="Search"]in isolation to fully cover the four-level selector chain.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/search-focus.test.ts` around lines 47 - 72, The selector precedence tests in getSearchInputFromContainer are not isolating each fallback, so matching for [cmdk-input], input[aria-label="Search TanStack"], input[aria-label="Search"], and input[type="search"] is ambiguous. Update the tests to use distinct mock inputs per selector and add dedicated cases that verify each step in the chain independently, especially the final type="search" fallback. Keep the assertions tied to getSearchInputFromContainer so each selector path is uniquely exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/SearchModal.tsx`:
- Around line 3358-3365: The scheduled focus logic in scheduleSearchInputFocus
can still run after the modal closes because it creates nested
requestAnimationFrame callbacks without any cancellation path. Update
scheduleSearchInputFocus to return a cleanup/cancel handle for the pending rAF
chain, then use that return value in both consuming effects that call it so
their cleanup cancels the scheduled focus instead of returning undefined. Make
sure the fix covers the SearchModal and AiDock callers that currently discard
cleanup, so stale focus cannot be applied to contentRef.current after close
transitions.
---
Nitpick comments:
In `@tests/search-focus.test.ts`:
- Around line 1-91: Add edge-case tests in search-focus.test.ts for
getSearchInputFromContainer’s early return when passed null and for the no-match
fallthrough when createMockContainer exposes none of the selectors. Reuse the
existing createMockContainer and createMockInput helpers, and assert the null
case returns null and the no-match container also returns null. Keep the new
coverage alongside the current getSearchInputFromContainer and
focusSearchInputInContainer assertions so the selector behavior is fully
exercised.
- Around line 47-72: The selector precedence tests in
getSearchInputFromContainer are not isolating each fallback, so matching for
[cmdk-input], input[aria-label="Search TanStack"], input[aria-label="Search"],
and input[type="search"] is ambiguous. Update the tests to use distinct mock
inputs per selector and add dedicated cases that verify each step in the chain
independently, especially the final type="search" fallback. Keep the assertions
tied to getSearchInputFromContainer so each selector path is uniquely exercised.
🪄 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: 0e51ca2b-f0c6-4f10-b973-1c2dfbbb1730
📒 Files selected for processing (4)
src/components/SearchModal.tsxsrc/contexts/SearchContext.tsxsrc/utils/searchFocus.tstests/search-focus.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
input[type="search"], but the cmdk search field renders as a text input with thecmdk-inputattribute.onOpenAutoFocus.Test plan
pnpm test:unit(includes newsearch-focustests)Summary by CodeRabbit