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 搜索Modal 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. Changes搜索 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 搜索Modal
participant schedule搜索InputFocus
participant focus搜索InputInContainer
RadixDialog->>搜索Modal: onOpenAutoFocus
搜索Modal->>搜索Modal: preventDefault()
搜索Modal->>schedule搜索InputFocus: contentRef.current
schedule搜索InputFocus->>schedule搜索InputFocus: nested requestAnimationFrame
schedule搜索InputFocus->>focus搜索InputInContainer: container
focus搜索InputInContainer->>focus搜索InputInContainer: 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 |
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
get搜索InputFromContainer(null)(the early-return branch inget搜索InputFromContainer) 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 get搜索InputFromContainer’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 get搜索InputFromContainer and focus搜索InputInContainer 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="搜索 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="搜索"]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="搜索 TanStack"]': cmdkInput, - 'input[aria-label="搜索"]': null, - 'input[type="search"]': createMockInput({ type: 'search' }), -}) +const cmdkContainer = createMockContainer({ + '[cmdk-input]': cmdkInput, + 'input[aria-label="搜索 TanStack"]': null, + 'input[aria-label="搜索"]': null, + 'input[type="search"]': null, +}) ... -const searchTypeContainer = createMockContainer({ - '[cmdk-input]': null, - 'input[aria-label="搜索 TanStack"]': null, - 'input[aria-label="搜索"]': searchTypeInput, - 'input[type="search"]': searchTypeInput, -}) +const searchTypeContainer = createMockContainer({ + '[cmdk-input]': null, + 'input[aria-label="搜索 TanStack"]': null, + 'input[aria-label="搜索"]': null, + 'input[type="search"]': searchTypeInput, +})Consider adding a dedicated case for
input[aria-label="搜索 TanStack"]andinput[aria-label="搜索"]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 get搜索InputFromContainer are not isolating each fallback, so matching for [cmdk-input], input[aria-label="搜索 TanStack"], input[aria-label="搜索"], 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 get搜索InputFromContainer 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/搜索Modal.tsx`:
- Around line 3358-3365: The scheduled focus logic in schedule搜索InputFocus
can still run after the modal closes because it creates nested
requestAnimationFrame callbacks without any cancellation path. Update
schedule搜索InputFocus 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 搜索Modal 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
get搜索InputFromContainer’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 get搜索InputFromContainer and
focus搜索InputInContainer assertions so the selector behavior is fully
exercised.
- Around line 47-72: The selector precedence tests in
get搜索InputFromContainer are not isolating each fallback, so matching for
[cmdk-input], input[aria-label="搜索 TanStack"], input[aria-label="搜索"],
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 get搜索InputFromContainer 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/搜索Modal.tsxsrc/contexts/搜索Context.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
注册 for free
to join this conversation on GitHub.
Already have an account?
登录 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