fix: added clear search on removing badge and added keyboard shortcuts #9438
Conversation
WalkthroughThe pull request modifies the Changes
Sequence DiagramsequenceDiagram
participant User
participant SearchComponent
participant InputField
User->>SearchComponent: Press Ctrl+K or Meta+K
SearchComponent->>InputField: Focus input field
User->>SearchComponent: Press Escape
alt Input not empty
SearchComponent->>InputField: Clear input
end
User->>SearchComponent: Select option
SearchComponent->>SearchComponent: Reset error state
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 1
🧹 Nitpick comments (1)
src/components/Common/SearchByMultipleFields.tsx (1)
Line range hint
1-283: Add test coverage for keyboard shortcuts and search clearing.The new keyboard shortcuts and search clearing functionality should be covered by tests to ensure reliability.
Would you like me to help generate test cases for:
- Keyboard shortcuts (Ctrl+K, Escape, etc.)
- Search clearing behavior when badges are removed
- Error handling with different field types
This will help maintain the stability of these new features.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/SearchByMultipleFields.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Common/SearchByMultipleFields.tsx
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
src/components/Common/SearchByMultipleFields.tsx (1)
116-123: 🛠️ Refactor suggestion
Consolidate Escape key handling and document keyboard shortcuts.
The Escape key handling is duplicated in the code. Also, keyboard shortcuts should be documented and made visible to users.
- Consolidate Escape key handling:
useEffect(() => {
const handleKeyDown = (e: KeyboardEvent) => {
if (
e.key === "/" &&
!(document.activeElement instanceof HTMLInputElement)
) {
e.preventDefault();
setOpen(true);
}
if ((e.ctrlKey || e.metaKey) && e.key === "k") {
e.preventDefault();
inputRef.current?.focus();
setOpen(false);
}
- if (e.key === "Escape") {
- setSearchValue("");
- }
if (open) {
if (e.key === "ArrowDown") {
setFocusedIndex((prevIndex) =>
prevIndex === options.length - 1 ? 0 : prevIndex + 1,
);
} else if (e.key === "ArrowUp") {
setFocusedIndex((prevIndex) =>
prevIndex === 0 ? options.length - 1 : prevIndex - 1,
);
} else if (e.key === "Enter") {
handleOptionChange(focusedIndex);
}
if (e.key === "Escape") {
- inputRef.current?.focus();
- setOpen(false);
+ setSearchValue("");
+ setOpen(false);
+ inputRef.current?.focus();
}- Add keyboard shortcuts to the interface:
interface SearchByMultipleFieldsProps {
// ... existing props
/** Keyboard shortcuts:
* - "/" to open search options
* - "Ctrl+K" or "Cmd+K" to focus search input
* - "Escape" to clear search and close options
* - "ArrowUp/Down" to navigate options
* - "Enter" to select option
*/
}Consider adding a tooltip or help text to display available keyboard shortcuts to users. This would improve discoverability of these features.
Let's check for potential shortcut conflicts:
✅ Verification successful
Let me check the keyboard event handlers in these files to verify potential conflicts.
Consolidate Escape key handling and document keyboard shortcuts
The keyboard shortcuts in SearchByMultipleFields are safe to implement as suggested:
- FilePreviewDialog only uses Arrow keys for navigation in a modal context
- NotificationsList doesn't expose any conflicting keyboard handlers
- The Ctrl+K/Cmd+K and Escape shortcuts are scoped appropriately and won't interfere with other components
Proceed with the suggested changes:
- Consolidate the Escape key handling to avoid duplication
- Add keyboard shortcut documentation to the component interface
- Consider adding a tooltip for better discoverability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other keyboard event handlers in the codebase
rg -A 5 "useEffect.*keydown|addEventListener.*keydown"
Length of output: 1595
Script:
#!/bin/bash
# Check keyboard event handling in NotificationsList
ast-grep --pattern 'const handleKeyDown = (e: $_) => { $$$ }' src/components/Notifications/NotificationsList.tsx
# Check keyboard event handling in FilePreviewDialog
ast-grep --pattern 'const handleKeyDown = (e: $_) => { $$$ }' src/components/Common/FilePreviewDialog.tsx
Length of output: 942
| if (clearSearch?.params && clearSearch?.params.includes("search")) { | ||
| setSearchValue(""); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor clearSearch logic for better efficiency.
The current implementation has redundant logic for clearing the search value. Let's simplify it.
useEffect(() => {
if (clearSearch?.value) {
- const clearinput = options
- .map((op) => op.key)
- .some((element) => clearSearch.params?.includes(element));
- clearinput ? setSearchValue("") : null;
inputRef.current?.focus();
- if (clearSearch?.params && clearSearch?.params.includes("search")) {
+ if (clearSearch?.params?.some(param =>
+ param === "search" || options.some(op => op.key === param)
+ )) {
setSearchValue("");
}
}
}, [clearSearch]);📝 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.
| if (clearSearch?.params && clearSearch?.params.includes("search")) { | |
| setSearchValue(""); | |
| } | |
| useEffect(() => { | |
| if (clearSearch?.value) { | |
| inputRef.current?.focus(); | |
| if (clearSearch?.params?.some(param => | |
| param === "search" || options.some(op => op.key === param) | |
| )) { | |
| setSearchValue(""); | |
| } | |
| } | |
| }, [clearSearch]); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Try this implement @noufalrahim
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes