fix: disable search auto-focus on mobile/touch devices#4128
fix: disable search auto-focus on mobile/touch devices#4128kasya merged 10 commits intoOWASP:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a media-query-driven hook Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/MultiSearch.tsx (1)
162-178:⚠️ Potential issue | 🟠 MajorAutofocus can be skipped when
isLoadedchanges after mount.The effect only depends on
shouldAutoFocus. If input is not mounted yet (isLoaded=false), focus is missed and not retried when loading completes.💡 Suggested fix
useEffect(() => { - if(shouldAutoFocus){ + if (isLoaded && shouldAutoFocus) { inputRef.current?.focus() } @@ - }, [shouldAutoFocus]) + }, [isLoaded, shouldAutoFocus])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/MultiSearch.tsx` around lines 162 - 178, The effect in MultiSearch.tsx that focuses the input and attaches the outside-click handler currently only depends on shouldAutoFocus, so if the input mounts later (isLoaded toggles) the focus is missed; update the useEffect dependencies to include isLoaded and run the focus logic only when isLoaded is true (use inputRef.current?.focus()), and keep the click-outside handler behavior intact (attach/remove document listener as before) so that focus is retried after loading completes; reference the useEffect block that uses shouldAutoFocus, isLoaded, inputRef, searchBarRef, and setShowSuggestions.
🤖 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 `@frontend/src/components/MultiSearch.tsx`:
- Around line 162-178: The effect in MultiSearch.tsx that focuses the input and
attaches the outside-click handler currently only depends on shouldAutoFocus, so
if the input mounts later (isLoaded toggles) the focus is missed; update the
useEffect dependencies to include isLoaded and run the focus logic only when
isLoaded is true (use inputRef.current?.focus()), and keep the click-outside
handler behavior intact (attach/remove document listener as before) so that
focus is retried after loading completes; reference the useEffect block that
uses shouldAutoFocus, isLoaded, inputRef, searchBarRef, and setShowSuggestions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/__tests__/unit/components/Search.test.tsxfrontend/src/components/MultiSearch.tsxfrontend/src/components/Search.tsxfrontend/src/hooks/useShouldAutoFocusSearch.ts
There was a problem hiding this comment.
1 issue found across 4 files
Confidence score: 4/5
- This PR appears safe to merge; the only noted issue is a minor test weakness rather than a product behavior change.
- In
frontend/__tests__/unit/components/Search.test.tsx, the newexpect(input).toHaveFocus()doesn’t actually verify refocus behavior because the input is already focused on mount whenuseShouldAutoFocusSearchis true. - Pay close attention to
frontend/__tests__/unit/components/Search.test.tsx- the focus assertion may not be exercising the intended refocus path.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/__tests__/unit/components/Search.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/Search.test.tsx:166">
P2: The added `expect(input).toHaveFocus()` does not actually test refocus behavior. With `useShouldAutoFocusSearch` mocked to true, the input is already focused on mount, and `fireEvent.click` doesn’t move focus to the button. The input stays focused regardless of whether the clear handler calls `focus()`, so this assertion will pass even if refocus logic is removed. Consider blurring the input (or using userEvent to change focus) before asserting refocus so the test can catch regressions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/__tests__/unit/components/Search.test.tsx`:
- Around line 171-177: The test queries the clear button with
container.querySelector which may return null and is clicked unguarded; update
the test in Search.test.tsx so the clearButton is checked before calling
fireEvent.click — either assert/expect that clearButton is truthy (or throw a
descriptive error) or prefer a non-null query (e.g., getByRole/getByLabelText)
to retrieve the button; ensure the change is applied to the test case using the
clearButton variable in the it('does not refocus input after clear when on
mobile') block and keep useShouldAutoFocusSearch and SearchBar usage unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/Search.test.tsxfrontend/src/components/MultiSearch.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/MultiSearch.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/__tests__/unit/components/Search.test.tsx (1)
174-184:⚠️ Potential issue | 🟡 MinorGuard nullable
clearButtonand assert focus precondition in the mobile clear test.On Line 181,
fireEvent.click(clearButton)is unguarded after a nullablequerySelector. Also, this test should explicitly prove the input is unfocused before clear; otherwise “does not refocus” can pass vacuously.Proposed test-hardening patch
it('does not refocus input after clear when on mobile', async () => { ;(useShouldAutoFocusSearch as jest.Mock).mockReturnValue(false) const { container } = render(<SearchBar {...defaultProps} isLoaded={true} />) const input = screen.getByPlaceholderText('Search projects...') fireEvent.change(input, { target: { value: 'test' } }) const clearButton = container.querySelector('button.absolute.rounded-md[class*="right-2"]') expect(clearButton).toBeInTheDocument() - fireEvent.click(clearButton) + input.blur() + expect(input).not.toHaveFocus() + if (!clearButton) { + throw new Error('Clear button not found') + } + fireEvent.click(clearButton) expect(input).toHaveValue('') // NOSONAR: Verifies input is not refocused on mobile when shouldAutoFocus is false expect(input).not.toHaveFocus() })#!/bin/bash # Verify nullable querySelector usages and unguarded clearButton click sites in this test file. rg -nP -C2 "const clearButton = container\.querySelector\(|fireEvent\.click\(clearButton\)" frontend/__tests__/unit/components/Search.test.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/Search.test.tsx` around lines 174 - 184, The test should guard the nullable clearButton and assert a precondition that the input is focused before clicking to ensure "does not refocus" isn't passing vacuously: update the 'does not refocus input after clear when on mobile' test for SearchBar by (1) asserting the input is focused (or programmatically focusing it) before the clear click so we have a meaningful precondition, (2) null-checking clearButton (the value returned from container.querySelector) and failing the test early if it's missing, and (3) then performing the click and asserting input.value is '' and input does not have focus; keep references to clearButton, input, and the SearchBar render in the same test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/__tests__/unit/components/Search.test.tsx`:
- Around line 174-184: The test should guard the nullable clearButton and assert
a precondition that the input is focused before clicking to ensure "does not
refocus" isn't passing vacuously: update the 'does not refocus input after clear
when on mobile' test for SearchBar by (1) asserting the input is focused (or
programmatically focusing it) before the clear click so we have a meaningful
precondition, (2) null-checking clearButton (the value returned from
container.querySelector) and failing the test early if it's missing, and (3)
then performing the click and asserting input.value is '' and input does not
have focus; keep references to clearButton, input, and the SearchBar render in
the same test.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
frontend/__tests__/unit/components/Search.test.tsx (3)
347-348: Inconsistent clear button query pattern; unguarded null access.This test still uses
container.querySelectorwithout a null guard, while the updated tests at lines 163 and 178 use the more robustscreen.getByRole('button', { name: 'Clear search' }). Consider aligning this test with the improved pattern for consistency and better error messages on failure.Suggested refactor
- const clearButton = container.querySelector('button.absolute.rounded-md[class*="right-2"]') - fireEvent.click(clearButton) + const clearButton = screen.getByRole('button', { name: 'Clear search' }) + fireEvent.click(clearButton)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/Search.test.tsx` around lines 347 - 348, The test is querying the clear button with container.querySelector and then clicking it without guarding for null; replace the query with the accessible query used elsewhere: use screen.getByRole('button', { name: 'Clear search' }) to find the button and then call fireEvent.click on that element (replace references to clearButton and the container.querySelector invocation), which removes the unguarded null access and yields better failure messages.
380-381: Same inconsistent query pattern in keyboard event tests.These keyboard accessibility tests also use
container.querySelectorwithout guards. For consistency with the refactored clear button tests, consider usingscreen.getByRole('button', { name: 'Clear search' })throughout this describe block.Suggested refactor for all three tests
- const clearButton = container.querySelector('button.absolute.rounded-md[class*="right-2"]') + const clearButton = screen.getByRole('button', { name: 'Clear search' })Also applies to: 395-396, 410-411
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/Search.test.tsx` around lines 380 - 381, Replace the fragile container.querySelector usage in the keyboard accessibility tests with an accessible query: use screen.getByRole('button', { name: 'Clear search' }) to locate the clear button (instead of container.querySelector('button.absolute.rounded-md[class*="right-2"]')) and then call fireEvent.keyDown on that element; update all occurrences in this describe block (including the instances referenced around lines ~395 and ~410) so the tests consistently use getByRole for the clear button and will fail loudly if the control is missing.
126-131: Minor formatting inconsistency.Line 126 is missing a space before the arrow function, unlike other tests in this file (e.g., line 119).
Formatting fix
- it('should NOT auto-focus on mobile/touch devices',()=>{ + it('should NOT auto-focus on mobile/touch devices', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/Search.test.tsx` around lines 126 - 131, The test "should NOT auto-focus on mobile/touch devices" has a formatting inconsistency: add a space before the arrow in the anonymous function declaration so it matches other tests (change the it call in the Search.test.tsx where useShouldAutoFocusSearch is mocked and SearchBar is rendered to use ", () => {" instead of ",()=>{"). Ensure the same spacing style is used for that it(...) callback to mirror the surrounding tests (see the useShouldAutoFocusSearch mock, render(<SearchBar ... />) and expect assertions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/__tests__/unit/components/Search.test.tsx`:
- Around line 347-348: The test is querying the clear button with
container.querySelector and then clicking it without guarding for null; replace
the query with the accessible query used elsewhere: use
screen.getByRole('button', { name: 'Clear search' }) to find the button and then
call fireEvent.click on that element (replace references to clearButton and the
container.querySelector invocation), which removes the unguarded null access and
yields better failure messages.
- Around line 380-381: Replace the fragile container.querySelector usage in the
keyboard accessibility tests with an accessible query: use
screen.getByRole('button', { name: 'Clear search' }) to locate the clear button
(instead of
container.querySelector('button.absolute.rounded-md[class*="right-2"]')) and
then call fireEvent.keyDown on that element; update all occurrences in this
describe block (including the instances referenced around lines ~395 and ~410)
so the tests consistently use getByRole for the clear button and will fail
loudly if the control is missing.
- Around line 126-131: The test "should NOT auto-focus on mobile/touch devices"
has a formatting inconsistency: add a space before the arrow in the anonymous
function declaration so it matches other tests (change the it call in the
Search.test.tsx where useShouldAutoFocusSearch is mocked and SearchBar is
rendered to use ", () => {" instead of ",()=>{"). Ensure the same spacing style
is used for that it(...) callback to mirror the surrounding tests (see the
useShouldAutoFocusSearch mock, render(<SearchBar ... />) and expect assertions).
|
@ItsImran10140 Hi! were you able yo reproduce this issue originally on the iOS device you used in the simulator? With the keyboard popping up on loading Chapters page? |
anurag2787
left a comment
There was a problem hiding this comment.
Hi @ItsImran10140 Thanks for working on it!
It is working fine Just a small suggestion ⬇️
|
Hi @kasya @anurag2787 I don't know In my physical android device I am seeing this behaviour and I don't have Iphone physically present So used the emulator and the their is also it showing. I can not say about the physical device. you can see what its showing. Thank you for the guide. |
hi @anurag2787 ! I think you forgot to submit your suggestion 👀 |
|
@MohdWaqar98 could you also resolve conflicts? We no longer have I'm still not able to reproduce this on a simulator 🤔 |
Oh sorry about that 😅 
Hi @ItsImran10140 please refer to this |
- Add useShouldAutoFocusSearch hook to detect mobile and touch devices - Update Search and MultiSearch components to conditionally auto-focus - Add tests for mobile auto-focus behavior - Fixes keyboard opening immediately on mobile when visiting /chapters and other search pages
ce01444 to
3d386e8
Compare
|
Hi @kasya @anurag2787 Thank you for the Guide. I have done what think is good can you review it and give some feedback. |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4128 +/- ##
==========================================
- Coverage 99.48% 99.47% -0.02%
==========================================
Files 520 520
Lines 16305 16311 +6
Branches 2204 2207 +3
==========================================
+ Hits 16221 16225 +4
Misses 56 56
- Partials 28 30 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
kasya
left a comment
There was a problem hiding this comment.
@ItsImran10140 thanks for working on this! 👍🏼
I pushed a small update. Please run make check-test before every push to your PR, to make sure all checks are passing. They were failing this last time.
Proposed change
Disable search auto-focus on mobile/touch devices
Resolves #3182
useShouldAutoFocusSearchhook that detects small screens (<768px) and touch devices (pointer: coarse)Search.tsxto conditionally auto-focus based on device typeMultiSearch.tsx(homepage) with same logiciPhone-13-PRO-localhost-3mep_uwoc1mnur.webm
Checklist
make check-testlocally: all warnings addressed, tests passed