Fix: Loses focus on User Search in users tab #9500
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (6)src/components/Users/ManageUsers.tsx (4)Line range hint The changes properly set up the new tab and search functionality with appropriate type safety.
The simplified loading state handling and reduced props in UserListView help prevent unnecessary re-renders, which was the root cause of the focus loss issue.
The SearchByMultipleFields component is properly implemented with:
The Tabs component is well-implemented with:
src/components/Users/UserListAndCard.tsx (2)
The UserListViewProps interface has been correctly simplified by:
Line range hint The UserListView component has been properly simplified to focus solely on view switching, which helps prevent unnecessary re-renders and focus loss issues. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/Users/UserListAndCard.tsx(2 hunks)
🔇 Additional comments (1)
src/components/Users/UserListAndCard.tsx (1)
3-3: LGTM: Clean import addition
The useEffect import is properly placed with other React imports.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Form/SearchInput.tsx (4)
26-36: Add JSDoc documentation for the component.The component's props and behavior should be documented for better maintainability. Consider adding JSDoc comments explaining the purpose of each prop, especially
secondaryandhotkeyrelated props.+/** + * A search input component with debounce, keyboard shortcuts, and focus management. + * @param props.debouncePeriod - Delay in ms before triggering onChange (default: 500) + * @param props.secondary - When true, disables global keyboard shortcuts + * @param props.hotkey - Custom keyboard shortcut keys + * @param props.hotkeyIcon - Custom keyboard shortcut icon + */ const SearchInput = forwardRef<HTMLInputElement, SearchInputProps>(
37-48: Use optional chaining for the onChange callback.The onChange callback should use optional chaining for better safety.
- () => onChange && onChange({ name, value: value || "" }), + () => onChange?.({ name, value: value || "" }),🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
68-80: Consider extracting keyboard shortcuts into constants.For better maintainability and reusability, consider extracting the keyboard shortcuts into named constants.
+const KEYBOARD_SHORTCUTS = { + CLEAR: ["Escape"], + FOCUS: { + MAC: ["Meta", "K"], + WINDOWS: ["Control", "K"] + } +} as const; useKeyboardShortcut( - ["Escape"], + KEYBOARD_SHORTCUTS.CLEAR, () => {
106-112: Fix the closing tag indentation.There's a minor formatting issue with the closing tag of the trailing focused element.
<div className="absolute inset-y-0 right-0 hidden gap-1 py-1.5 pr-1.5 md:flex"> <kbd className="inline-flex items-center rounded border border-secondary-200 bg-white px-2 font-sans text-sm font-medium text-secondary-500"> Esc </kbd> - </div> + </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Form/SearchInput.tsx(2 hunks)src/components/Users/UserListAndCard.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Users/UserListAndCard.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Form/SearchInput.tsx
[error] 43-43: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
src/components/Form/SearchInput.tsx (3)
49-55: LGTM! Focus management is well-implemented.
The focus management implementation properly handles:
- Both forwarded and internal refs
- Platform-specific keyboard shortcuts (Meta for Mac, Control for Windows)
- Secondary mode to disable global shortcuts
57-66: LGTM! Keyboard shortcut UI is accessible and platform-aware.
The implementation properly handles:
- Platform-specific shortcut display
- Semantic HTML with kbd elements
- Consistent styling
26-36: Verify the focus retention in the User Search.
The implementation looks correct for fixing the focus loss issue. The component now properly handles ref forwarding and focus management. Please verify that:
- Focus is retained when the search value changes
- Focus works correctly with keyboard shortcuts
- Focus behavior is consistent across different browsers
Also applies to: 89-89
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Form/SearchInput.tsx (3)
46-46: Consider optional chaining for cleaner code.Static analysis suggests using optional chaining instead of “onChange && onChange(…)”. This yields slightly cleaner, more idiomatic TypeScript.
Proposed change for line 46:
- onChange && onChange({ name, value: value || "" }); + onChange?.({ name, value: value || "" });🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 eslint
[error] 46-46: Delete
··(prettier/prettier)
93-101: Extract repeated keyboard shortcut UI.You are displaying the hotkey with a similar inline “kbd” element. Consider extracting a small, reusable keyboard shortcut UI or component to ensure consistency throughout the codebase and reduce duplication.
🧰 Tools
🪛 eslint
[error] 93-93: Delete
··(prettier/prettier)
[error] 94-94: Replace
············with··········(prettier/prettier)
[error] 95-95: Delete
··(prettier/prettier)
[error] 96-96: Delete
··(prettier/prettier)
[error] 97-97: Replace
··················with················(prettier/prettier)
[error] 98-98: Delete
··(prettier/prettier)
[error] 99-99: Delete
··(prettier/prettier)
[error] 100-100: Replace
············with··········(prettier/prettier)
[error] 101-101: Delete
··(prettier/prettier)
26-116: Address lint warnings regarding indentation.A large number of “prettier/prettier” errors were flagged, mostly related to indentation. Please format the code consistently to adhere to Prettier rules, ensuring a cleaner diff and consistency across the codebase.
🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 eslint
[error] 26-26: Delete
··(prettier/prettier)
[error] 27-27: Delete
··(prettier/prettier)
[error] 28-28: Delete
··(prettier/prettier)
[error] 29-29: Delete
··(prettier/prettier)
[error] 30-30: Delete
··(prettier/prettier)
[error] 31-31: Delete
··(prettier/prettier)
[error] 32-32: Delete
··(prettier/prettier)
[error] 33-33: Delete
··(prettier/prettier)
[error] 34-34: Delete
··(prettier/prettier)
[error] 35-35: Delete
··(prettier/prettier)
[error] 36-36: Replace
····with··(prettier/prettier)
[error] 37-37: Replace
······with····(prettier/prettier)
[error] 38-38: Delete
··(prettier/prettier)
[error] 39-39: Replace
··const·internalRef·=·useRef<HTMLInputElement>(null);·withconst·internalRef·=·useRef<HTMLInputElement>(null);(prettier/prettier)
[error] 40-40: Delete
··(prettier/prettier)
[error] 41-41: Delete
··(prettier/prettier)
[error] 42-42: Delete
··(prettier/prettier)
[error] 43-43: Delete
··(prettier/prettier)
[error] 44-44: Delete
··(prettier/prettier)
[error] 45-45: Delete
··(prettier/prettier)
[error] 46-46: Delete
··(prettier/prettier)
[error] 47-47: Delete
··(prettier/prettier)
[error] 48-48: Delete
··(prettier/prettier)
[error] 49-49: Delete
··(prettier/prettier)
[error] 50-50: Delete
··(prettier/prettier)
[error] 51-51: Replace
······with····(prettier/prettier)
[error] 52-52: Delete
··(prettier/prettier)
[error] 53-53: Delete
··(prettier/prettier)
[error] 54-54: Replace
······with····(prettier/prettier)
[error] 55-55: Delete
··(prettier/prettier)
[error] 56-56: Delete
··(prettier/prettier)
[error] 57-57: Replace
··········with········(prettier/prettier)
[error] 58-58: Delete
··(prettier/prettier)
[error] 59-59: Delete
··(prettier/prettier)
[error] 60-60: Delete
··(prettier/prettier)
[error] 61-61: Delete
··(prettier/prettier)
[error] 62-62: Delete
··(prettier/prettier)
[error] 63-63: Delete
··(prettier/prettier)
[error] 64-64: Delete
··(prettier/prettier)
[error] 65-65: Delete
··(prettier/prettier)
[error] 66-66: Delete
··(prettier/prettier)
[error] 67-67: Delete
··(prettier/prettier)
[error] 68-68: Delete
··(prettier/prettier)
[error] 69-69: Delete
··(prettier/prettier)
[error] 70-70: Delete
··(prettier/prettier)
[error] 71-71: Replace
··········with········(prettier/prettier)
[error] 72-72: Delete
··(prettier/prettier)
[error] 73-73: Delete
··(prettier/prettier)
[error] 74-74: Replace
··········with········(prettier/prettier)
[error] 75-75: Delete
··(prettier/prettier)
[error] 76-76: Replace
······with····(prettier/prettier)
[error] 77-77: Delete
··(prettier/prettier)
[error] 78-78: Delete
··(prettier/prettier)
[error] 79-79: Delete
··(prettier/prettier)
[error] 80-80: Replace
··········with········(prettier/prettier)
[error] 81-81: Delete
··(prettier/prettier)
[error] 82-82: Delete
··(prettier/prettier)
[error] 83-83: Delete
··(prettier/prettier)
[error] 84-84: Delete
··(prettier/prettier)
[error] 85-85: Delete
··(prettier/prettier)
[error] 86-86: Delete
··(prettier/prettier)
[error] 87-87: Delete
··(prettier/prettier)
[error] 88-88: Delete
··(prettier/prettier)
[error] 89-89: Delete
··(prettier/prettier)
[error] 90-90: Delete
··(prettier/prettier)
[error] 91-91: Delete
··(prettier/prettier)
[error] 92-92: Replace
··········with········(prettier/prettier)
[error] 93-93: Delete
··(prettier/prettier)
[error] 94-94: Replace
············with··········(prettier/prettier)
[error] 95-95: Delete
··(prettier/prettier)
[error] 96-96: Delete
··(prettier/prettier)
[error] 97-97: Replace
··················with················(prettier/prettier)
[error] 98-98: Delete
··(prettier/prettier)
[error] 99-99: Delete
··(prettier/prettier)
[error] 100-100: Replace
············with··········(prettier/prettier)
[error] 101-101: Delete
··(prettier/prettier)
[error] 102-102: Delete
··(prettier/prettier)
[error] 103-103: Delete
··(prettier/prettier)
[error] 104-104: Delete
··(prettier/prettier)
[error] 105-105: Delete
··(prettier/prettier)
[error] 106-106: Delete
··(prettier/prettier)
[error] 107-107: Delete
··(prettier/prettier)
[error] 108-108: Delete
··(prettier/prettier)
[error] 109-109: Delete
··(prettier/prettier)
[error] 110-110: Delete
··(prettier/prettier)
[error] 111-111: Replace
········with······(prettier/prettier)
[error] 112-112: Delete
··(prettier/prettier)
[error] 113-113: Delete
··(prettier/prettier)
[error] 114-114: Delete
··(prettier/prettier)
[error] 115-115: Delete
··(prettier/prettier)
[error] 116-116: Delete
··(prettier/prettier)
🪛 GitHub Check: lint
[failure] 26-26:
Delete··
[failure] 27-27:
Delete··
[failure] 28-28:
Delete··
[failure] 29-29:
Delete··
[failure] 30-30:
Delete··
[failure] 31-31:
Delete··
[failure] 32-32:
Delete··
[failure] 33-33:
Delete··
[failure] 34-34:
Delete··
[failure] 35-35:
Delete··
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Form/SearchInput.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Form/SearchInput.tsx
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 eslint
src/components/Form/SearchInput.tsx
[error] 26-26: Delete ··
(prettier/prettier)
[error] 27-27: Delete ··
(prettier/prettier)
[error] 28-28: Delete ··
(prettier/prettier)
[error] 29-29: Delete ··
(prettier/prettier)
[error] 30-30: Delete ··
(prettier/prettier)
[error] 31-31: Delete ··
(prettier/prettier)
[error] 32-32: Delete ··
(prettier/prettier)
[error] 33-33: Delete ··
(prettier/prettier)
[error] 34-34: Delete ··
(prettier/prettier)
[error] 35-35: Delete ··
(prettier/prettier)
[error] 36-36: Replace ···· with ··
(prettier/prettier)
[error] 37-37: Replace ······ with ····
(prettier/prettier)
[error] 38-38: Delete ··
(prettier/prettier)
[error] 39-39: Replace ··const·internalRef·=·useRef<HTMLInputElement>(null);· with const·internalRef·=·useRef<HTMLInputElement>(null);
(prettier/prettier)
[error] 40-40: Delete ··
(prettier/prettier)
[error] 41-41: Delete ··
(prettier/prettier)
[error] 42-42: Delete ··
(prettier/prettier)
[error] 43-43: Delete ··
(prettier/prettier)
[error] 44-44: Delete ··
(prettier/prettier)
[error] 45-45: Delete ··
(prettier/prettier)
[error] 46-46: Delete ··
(prettier/prettier)
[error] 47-47: Delete ··
(prettier/prettier)
[error] 48-48: Delete ··
(prettier/prettier)
[error] 49-49: Delete ··
(prettier/prettier)
[error] 50-50: Delete ··
(prettier/prettier)
[error] 51-51: Replace ······ with ····
(prettier/prettier)
[error] 52-52: Delete ··
(prettier/prettier)
[error] 53-53: Delete ··
(prettier/prettier)
[error] 54-54: Replace ······ with ····
(prettier/prettier)
[error] 55-55: Delete ··
(prettier/prettier)
[error] 56-56: Delete ··
(prettier/prettier)
[error] 57-57: Replace ·········· with ········
(prettier/prettier)
[error] 58-58: Delete ··
(prettier/prettier)
[error] 59-59: Delete ··
(prettier/prettier)
[error] 60-60: Delete ··
(prettier/prettier)
[error] 61-61: Delete ··
(prettier/prettier)
[error] 62-62: Delete ··
(prettier/prettier)
[error] 63-63: Delete ··
(prettier/prettier)
[error] 64-64: Delete ··
(prettier/prettier)
[error] 65-65: Delete ··
(prettier/prettier)
[error] 66-66: Delete ··
(prettier/prettier)
[error] 67-67: Delete ··
(prettier/prettier)
[error] 68-68: Delete ··
(prettier/prettier)
[error] 69-69: Delete ··
(prettier/prettier)
[error] 70-70: Delete ··
(prettier/prettier)
[error] 71-71: Replace ·········· with ········
(prettier/prettier)
[error] 72-72: Delete ··
(prettier/prettier)
[error] 73-73: Delete ··
(prettier/prettier)
[error] 74-74: Replace ·········· with ········
(prettier/prettier)
[error] 75-75: Delete ··
(prettier/prettier)
[error] 76-76: Replace ······ with ····
(prettier/prettier)
[error] 77-77: Delete ··
(prettier/prettier)
[error] 78-78: Delete ··
(prettier/prettier)
[error] 79-79: Delete ··
(prettier/prettier)
[error] 80-80: Replace ·········· with ········
(prettier/prettier)
[error] 81-81: Delete ··
(prettier/prettier)
[error] 82-82: Delete ··
(prettier/prettier)
[error] 83-83: Delete ··
(prettier/prettier)
[error] 84-84: Delete ··
(prettier/prettier)
[error] 85-85: Delete ··
(prettier/prettier)
[error] 86-86: Delete ··
(prettier/prettier)
[error] 87-87: Delete ··
(prettier/prettier)
[error] 88-88: Delete ··
(prettier/prettier)
[error] 89-89: Delete ··
(prettier/prettier)
[error] 90-90: Delete ··
(prettier/prettier)
[error] 91-91: Delete ··
(prettier/prettier)
[error] 92-92: Replace ·········· with ········
(prettier/prettier)
[error] 93-93: Delete ··
(prettier/prettier)
[error] 94-94: Replace ············ with ··········
(prettier/prettier)
[error] 95-95: Delete ··
(prettier/prettier)
[error] 96-96: Delete ··
(prettier/prettier)
[error] 97-97: Replace ·················· with ················
(prettier/prettier)
[error] 98-98: Delete ··
(prettier/prettier)
[error] 99-99: Delete ··
(prettier/prettier)
[error] 100-100: Replace ············ with ··········
(prettier/prettier)
[error] 101-101: Delete ··
(prettier/prettier)
[error] 102-102: Delete ··
(prettier/prettier)
[error] 103-103: Delete ··
(prettier/prettier)
[error] 104-104: Delete ··
(prettier/prettier)
[error] 105-105: Delete ··
(prettier/prettier)
[error] 106-106: Delete ··
(prettier/prettier)
[error] 107-107: Delete ··
(prettier/prettier)
[error] 108-108: Delete ··
(prettier/prettier)
[error] 109-109: Delete ··
(prettier/prettier)
[error] 110-110: Delete ··
(prettier/prettier)
[error] 111-111: Replace ········ with ······
(prettier/prettier)
[error] 112-112: Delete ··
(prettier/prettier)
[error] 113-113: Delete ··
(prettier/prettier)
[error] 114-114: Delete ··
(prettier/prettier)
[error] 115-115: Delete ··
(prettier/prettier)
[error] 116-116: Delete ··
(prettier/prettier)
🪛 GitHub Check: lint
src/components/Form/SearchInput.tsx
[failure] 26-26:
Delete ··
[failure] 27-27:
Delete ··
[failure] 28-28:
Delete ··
[failure] 29-29:
Delete ··
[failure] 30-30:
Delete ··
[failure] 31-31:
Delete ··
[failure] 32-32:
Delete ··
[failure] 33-33:
Delete ··
[failure] 34-34:
Delete ··
[failure] 35-35:
Delete ··
🔇 Additional comments (4)
src/components/Form/SearchInput.tsx (4)
26-36: Good use of forwardRef for improved focus handling.
Forwarding the ref allows external components to focus the input directly and effectively addresses the focus loss issues. This is aligned with React’s recommended approach and should help maintain focus on re-renders.
🧰 Tools
🪛 eslint
[error] 26-26: Delete ··
(prettier/prettier)
[error] 27-27: Delete ··
(prettier/prettier)
[error] 28-28: Delete ··
(prettier/prettier)
[error] 29-29: Delete ··
(prettier/prettier)
[error] 30-30: Delete ··
(prettier/prettier)
[error] 31-31: Delete ··
(prettier/prettier)
[error] 32-32: Delete ··
(prettier/prettier)
[error] 33-33: Delete ··
(prettier/prettier)
[error] 34-34: Delete ··
(prettier/prettier)
[error] 35-35: Delete ··
(prettier/prettier)
[error] 36-36: Replace ···· with ··
(prettier/prettier)
🪛 GitHub Check: lint
[failure] 26-26:
Delete ··
[failure] 27-27:
Delete ··
[failure] 28-28:
Delete ··
[failure] 29-29:
Delete ··
[failure] 30-30:
Delete ··
[failure] 31-31:
Delete ··
[failure] 32-32:
Delete ··
[failure] 33-33:
Delete ··
[failure] 34-34:
Delete ··
[failure] 35-35:
Delete ··
39-40: Clarify merged references.
Using the expression "(ref || internalRef)" seamlessly merges any external ref passed in with your internal ref handling. Just be sure to confirm that no code depends on the presence of the one that is not used. Otherwise, this logic and type assertion are correct.
🧰 Tools
🪛 eslint
[error] 39-39: Replace ··const·internalRef·=·useRef<HTMLInputElement>(null);· with const·internalRef·=·useRef<HTMLInputElement>(null);
(prettier/prettier)
[error] 40-40: Delete ··
(prettier/prettier)
53-63: Focus shortcut handling is appropriate.
The hotkey logic effectively focuses the input. Ensure that the "overrideSystem" option behaves as intended across various browsers, so system-level shortcuts (e.g. Ctrl+K to open a new tab in some contexts) do not conflict.
🧰 Tools
🪛 eslint
[error] 53-53: Delete ··
(prettier/prettier)
[error] 54-54: Replace ······ with ····
(prettier/prettier)
[error] 55-55: Delete ··
(prettier/prettier)
[error] 56-56: Delete ··
(prettier/prettier)
[error] 57-57: Replace ·········· with ········
(prettier/prettier)
[error] 58-58: Delete ··
(prettier/prettier)
[error] 59-59: Delete ··
(prettier/prettier)
[error] 60-60: Delete ··
(prettier/prettier)
[error] 61-61: Delete ··
(prettier/prettier)
[error] 62-62: Delete ··
(prettier/prettier)
[error] 63-63: Delete ··
(prettier/prettier)
64-76: Nice handling of the Escape key.
Clearing the input and blurring on Escape is a neat, user-friendly addition. Verify that losing focus on Escape aligns with typical user expectations.
🧰 Tools
🪛 eslint
[error] 64-64: Delete ··
(prettier/prettier)
[error] 65-65: Delete ··
(prettier/prettier)
[error] 66-66: Delete ··
(prettier/prettier)
[error] 67-67: Delete ··
(prettier/prettier)
[error] 68-68: Delete ··
(prettier/prettier)
[error] 69-69: Delete ··
(prettier/prettier)
[error] 70-70: Delete ··
(prettier/prettier)
[error] 71-71: Replace ·········· with ········
(prettier/prettier)
[error] 72-72: Delete ··
(prettier/prettier)
[error] 73-73: Delete ··
(prettier/prettier)
[error] 74-74: Replace ·········· with ········
(prettier/prettier)
[error] 75-75: Delete ··
(prettier/prettier)
[error] 76-76: Replace ······ with ····
(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Form/SearchInput.tsx (3)
26-36: Add component documentation and display nameThe refactor to use forwardRef is good. Consider adding:
- JSDoc documentation to describe the component's purpose and ref usage
- A display name for better debugging
+/** + * A search input component with debounce and keyboard shortcut support. + * Forwards ref to the underlying input element for focus management. + */ const SearchInput = forwardRef<HTMLInputElement, SearchInputProps>( ({ debouncePeriod = 500, className = "w-full md:max-w-sm", onChange, name = "search", ...props }: SearchInputProps, ref, ) => {); +SearchInput.displayName = 'SearchInput'; export default SearchInput;
43-51: Enhance debounce implementationThe debounce logic works correctly, but can be improved for better maintainability.
useEffect(() => { if (value !== props.value) { const timeoutId = setTimeout(() => { - onChange && onChange({ name, value: value || "" }); + onChange?.({ name, value: value || "" }); }, debouncePeriod); - return () => clearTimeout(timeoutId); + return () => { + clearTimeout(timeoutId); + }; } }, [value, debouncePeriod, name, onChange, props.value]);🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
95-98: Enhance keyboard shortcut accessibilityConsider adding ARIA labels to the keyboard shortcut indicators for better screen reader support.
<kbd className="inline-flex items-center rounded border border-secondary-200 bg-white px-2 font-sans text-sm font-medium text-secondary-500 focus:opacity-0" + aria-label="Search shortcut" > {props.hotkeyIcon || (isAppleDevice ? "⌘K" : "Ctrl+K")} </kbd> // ... and for the Escape key <kbd className="inline-flex items-center rounded border border-secondary-200 bg-white px-2 font-sans text-sm font-medium text-secondary-500" + aria-label="Clear search shortcut" > Esc </kbd>Also applies to: 103-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Form/SearchInput.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Form/SearchInput.tsx
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
src/components/Form/SearchInput.tsx (3)
37-41: LGTM: Clean ref management implementation
The implementation properly handles both external and internal refs, with good state initialization from props.
54-76: LGTM: Robust keyboard shortcut implementation
The keyboard shortcuts implementation is well thought out:
- Proper platform-specific shortcuts (⌘K vs Ctrl+K)
- Escape handling for clearing and blur
- Secondary mode prevention for nested contexts
Line range hint 1-114: Verify focus management in the users tab
The implementation looks solid for fixing the focus loss issue. To ensure complete resolution:
✅ Verification successful
Focus management implementation is correctly used in the users tab
The SearchInput component is properly implemented in UserListView with:
- Correct ref forwarding using
useRef - Proper focus management through
searchInputRef - Automatic focus when search value is present via useEffect
- Keyboard shortcuts inherited from the SearchInput component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper usage of the SearchInput component in the users tab
rg -A 5 "SearchInput.*ref" src/components/Users/
Length of output: 50
Script:
#!/bin/bash
# Let's first check for any usage of SearchInput in the Users directory
rg "SearchInput" src/components/Users/
# Also check for any imports from the Form directory in Users
rg "from.*Form" src/components/Users/
# Let's also check the actual implementation of SearchInput in any user-related components
ast-grep --pattern 'const $_ = () => {
$$$
<SearchInput
$$$
/>
$$$
}'
Length of output: 3888
Script:
#!/bin/bash
# Let's check the specific implementation in UserListAndCard.tsx
cat src/components/Users/UserListAndCard.tsx
Length of output: 13165
🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
I don’t think the Cypress tests failed due to the changes in this PR. |
rithviknishad
added
changes required
invalid
…m/AdityaJ2305/care_fe into issue/9398/search_bar_loses_focus
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Users/UserListAndCard.tsx (1)
351-353: Remove debugging console.log statementThe console.log statement for debugging re-renders should be removed before merging.
- useEffect(() => { - console.log("re-rednerd"); - }, []);src/components/Users/ManageUsers.tsx (1)
108-119: Simplify loading state handlingThe current implementation assigns
<Loading />tomanageUsersand then immediately reassigns it in the else block. This can be simplified.- if (userListLoading || districtDataLoading || !userListData) { - manageUsers = <Loading />; - } else { - manageUsers = ( + manageUsers = userListLoading || districtDataLoading || !userListData ? ( + <Loading /> + ) : ( <div> <UserListView users={userListData?.results ?? []} activeTab={activeTab} /> <Pagination totalCount={userListData.count} /> </div> ); - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Facility/FacilityUsers.tsx(4 hunks)src/components/Users/ManageUsers.tsx(3 hunks)src/components/Users/UserListAndCard.tsx(2 hunks)
🧰 Additional context used
🪛 eslint
src/components/Users/UserListAndCard.tsx
[error] 350-350: 't' is assigned a value but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: lint
src/components/Users/UserListAndCard.tsx
[failure] 350-350:
't' is assigned a value but never used. Allowed unused vars must match /^_/u
🔇 Additional comments (4)
src/components/Users/UserListAndCard.tsx (1)
349-349: LGTM: Props interface simplification
Moving search functionality out of this component is a good approach to prevent unnecessary re-renders that were causing focus loss issues.
src/components/Facility/FacilityUsers.tsx (1)
81-90: LGTM: SearchInput implementation
The SearchInput component is correctly implemented with proper id and onChange handler. This matches the pattern used in ManageUsers and helps fix the focus management issue.
src/components/Users/ManageUsers.tsx (2)
181-190: LGTM: SearchInput implementation
The SearchInput implementation matches the pattern in FacilityUsers and helps resolve the focus management issue by preventing unnecessary re-renders of the UserListView component.
183-189: Verify focus retention across components
The implementation looks good, with search functionality consistently implemented in both ManageUsers and FacilityUsers components. This should resolve the focus loss issue by preventing unnecessary re-renders.
Also applies to: 83-89
✅ Verification successful
Focus management implementation verified across components
The search functionality is consistently implemented in both ManageUsers and FacilityUsers components, using the same pattern:
- Both use SearchInput with identical props structure
- Both handle state updates through updateQuery
- Both maintain username in qParams state
This consistent implementation confirms that the focus management fix has been properly applied across all relevant components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent SearchInput implementation across components
# Check for any other instances of SearchInput that might need similar updates
rg --type tsx --type ts "SearchInput.*username.*onChange"
Length of output: 87
Script:
#!/bin/bash
# Search for SearchInput components with onChange handlers
rg -t ts -t typescript -t tsx "SearchInput.*onChange"
# Also check for any other instances of username-related search functionality
rg -t ts -t typescript -t tsx "search.*username|username.*search" -A 2 -B 2
Length of output: 203
Script:
#!/bin/bash
# Find all TypeScript/React files first
fd "\.(ts|tsx)$" src/components
# Then use these file patterns with ripgrep
rg "SearchInput.*onChange" $(fd "\.(ts|tsx)$" src/components)
# Search for username-related search functionality
rg "search.*username|username.*search" -A 2 -B 2 $(fd "\.(ts|tsx)$" src/components)
Length of output: 58633
AdityaJ2305
force-pushed
the
issue/9398/search_bar_loses_focus
branch
from
3c5156a to
55196d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Facility/FacilityUsers.tsx (2)
22-24: Initialize user list elements more cleanly.
Usinglet usersList = <></>;anduseState("cardView")foractiveTabis fine. However, consider consolidating your state initialization for clarity, especially if you anticipate future expansions for loading states or multi-tab logic.Also applies to: 29-29
55-67: Check error handling before rendering the user list.
Currently, you render<Loading />untiluserListDatabecomes available. In scenarios where API calls fail, there is no fallback UI for errors. Add an error state to avoid indefinite loading.src/components/Users/ManageUsers.tsx (2)
102-113: Add an error fallback for user/district data.
Similar to the feedback inFacilityUsers.tsx, consider adding an error state to prevent indefinite loading in edge cases where data fetching fails.
203-204: Avoid duplicating IDs.
The<div>withid="user-list-view"is nested inside the<TabsTrigger>block that also uses the sameid. Verify whether both references are needed or if one can be removed to prevent ambiguous selectors or potential flakiness in tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cypress/pageobject/Users/UserSearch.ts(1 hunks)package.json(1 hunks)src/components/Facility/FacilityUsers.tsx(4 hunks)src/components/Users/ManageUsers.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (5)
cypress/pageobject/Users/UserSearch.ts (1)
86-86: Confirm correct attribute usage for the active tab.
The switch from checking for a class to verifying "data-state"="active" is acceptable. Ensure that the corresponding element (id="user-list-view") is indeed updated when switching tabs so that this test consistently passes.
src/components/Facility/FacilityUsers.tsx (2)
5-12: Validate new imports and removed references.
You replaced old references with CareIcon, Loading, Tabs, and SearchByMultipleFields. Ensure redundant imports were removed and that these new components are fully integrated without leftover console warnings or code references to removed imports (e.g., old Tabs import).
82-116: Ensure the search input and tab switches address the focus issue (#9398).
Providing SearchByMultipleFields and Tabs is the right approach for consistent UI. Verify that switching tabs or performing a search does not cause focus to be lost on the input field if that’s a desired behavior from the PR objectives.
src/components/Users/ManageUsers.tsx (2)
9-15: New UI dependencies and reusable logic.
The new imports for Tabs, SearchByMultipleFields, and the shift to let usersList: JSX.Element = <></>; align well with the approach used in FacilityUsers.tsx. This maintains consistency across components, simplifying future maintenance.
Also applies to: 36-36, 41-41, 47-47
175-210: Centralize search and tab focus handling to address #9398.
The usage of SearchByMultipleFields and Tabs is a step toward resolving the user search focus issue. Confirm that the component retains focus appropriately when switching between Card and List views, or when the search input changes, in order to fully meet the PR objective of preventing focus loss.
rithviknishad
added
needs testing
needs review
and removed
changes required
Deploy-Failed
|
👋 Hi, @AdityaJ2305, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
no longer relevant in new architecture |
Proposed Changes
Fixes Search bar loses focus after screen rebuild in Users section #9398
Move out the SearchInput to the individual components that use UserListView.
Fix the loosing focus for
ManageUsers@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tabs,TabsList,TabsTrigger, andTabsContentfor better navigation.Bug Fixes