Fix : Clear search field when navigating back with browser controls #10777
Conversation
WalkthroughThe changes introduce new conditions and hooks in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/SearchByMultipleFields.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
| useEffect(() => { | ||
| if (inputRef.current) { | ||
| inputRef.current.focus(); | ||
| } | ||
| }, [selectedOptionIndex]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating focus management logic.
The new useEffect hook for focusing the input field might conflict with existing focus management code. The component already handles focus in multiple places:
- When changing options (line 121)
- When using keyboard shortcuts (line 145)
- When pressing Escape (line 150)
- When
autoFocusprop changes (line 236)
Consider consolidating focus management into a single location or using a more robust focus management solution to prevent potential issues:
- useEffect(() => {
- if (inputRef.current) {
- inputRef.current.focus();
- }
- }, [selectedOptionIndex]);
const handleOptionChange = useCallback(
(index: number) => {
setSelectedOptionIndex(index);
const option = options[index];
setSearchValue(option.value || "");
setFocusedIndex(options.findIndex((op) => op.key === option.key));
setOpen(false);
- inputRef.current?.focus();
setError(false);
onSearch(option.key, option.value);
onFieldChange?.(options[index]);
+ // Handle focus in a single place
+ requestAnimationFrame(() => {
+ inputRef.current?.focus();
+ });
},
[onSearch],
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (inputRef.current) { | |
| inputRef.current.focus(); | |
| } | |
| }, [selectedOptionIndex]); | |
| // (Removed the following useEffect hook) | |
| // useEffect(() => { | |
| // if (inputRef.current) { | |
| // inputRef.current.focus(); | |
| // } | |
| // }, [selectedOptionIndex]); | |
| const handleOptionChange = useCallback( | |
| (index: number) => { | |
| setSelectedOptionIndex(index); | |
| const option = options[index]; | |
| setSearchValue(option.value || ""); | |
| setFocusedIndex(options.findIndex((op) => op.key === option.key)); | |
| setOpen(false); | |
| // Handle focus in a single place | |
| requestAnimationFrame(() => { | |
| inputRef.current?.focus(); | |
| }); | |
| setError(false); | |
| onSearch(option.key, option.value); | |
| onFieldChange?.(options[index]); | |
| }, | |
| [onSearch], | |
| ); |
| useEffect(() => { | ||
| if (autoFocus) { | ||
| inputRef.current?.focus(); | ||
| } | ||
| }, [autoFocus, open, selectedOptionIndex]); |
There was a problem hiding this comment.
why was this moved to the top? let's keep the diff clean. avoid unnecessary changes. do a self-review
|
@rithviknishad @Jacobjeevan Any changes required ? |
Check Rithvik's comment above; checkout files changed tab and modify the files accordingly. |
|
Moved autoFocus useEffect to the top. |
|
🤔 Did you click on the link above? The point was to move useEffect back down to line 223 - 228 😅 |
|
🤔 I'm comparing this to care.ohc.network - what exactly is the difference in behavior here? I thought you were going to completely clear the results on clicking back button. |
|
@Jacobjeevan In care.ohc.network , When navigating back using the mouse back button, fix-23.mp4When using the browser's mouse back button, the 'Search Encounter' field should clear as if characters are being deleted one by one. fix-32.mp4 |
|
|
|
merge checklist not followed; |
|
@rithviknishad @Jacobjeevan Completed merge checklist, Can we re-open this PR ? |
|
Whats the status here @Jeffrin2005 |
|
@bodhish I have been working on this for the last three days, but I am unable to fix it. I will update it tomorrow itself. |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Ensure that UI text is kept in I18n files.
Prep screenshot or demo video for changelog entry, and attach it to issue.
v12.mp4
Summary by CodeRabbit
autoFocusprop is true.