Feat: Update the design and shortcut key for SearchByMultipleOptions #9851
Conversation
WalkthroughThis pull request introduces comprehensive modifications to the search functionality across multiple components. The primary focus is on enhancing the Changes
Assessment against linked issues
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 (1)
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. |
|
@AdityaJ2305 I think you can make use of |
|
Fixed: #9867 (comment) |
![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
🧹 Nitpick comments (3)
src/pages/Organization/OrganizationPatients.tsx (1)
41-52: Consider improving phone number validation.The phone number validation logic could be more robust:
- The magic number
13should be extracted as a constant for better maintainability.- The validation should consider minimum length requirements for different phone number formats.
Apply this diff to improve the validation:
+const MIN_PHONE_LENGTH = 13; + const handleSearch = useCallback((key: string, value: string) => { const searchParams = { name: key === "name" ? value : "", phone_number: key === "phone_number" - ? value.length >= 13 || value === "" + ? value.length >= MIN_PHONE_LENGTH || value === "" ? value : undefined : undefined, }; updateQuery(searchParams); }, []);src/components/Common/SearchByMultipleFields.tsx (2)
191-214: Reduce code duplication in keyboard shortcut hints.The keyboard shortcut hint UI is duplicated between the phone number input and text input sections. Consider extracting it into a reusable component.
Create a new component for the keyboard hints:
const KeyboardShortcutHint: React.FC<{ isOpen: boolean }> = ({ isOpen }) => { if (isOpen) { return ( <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> ESC </span> ); } return ( <span> {isAppleDevice ? ( <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> ⌘K </span> ) : ( <div className="flex gap-1 font-medium"> <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> Ctrl </span> <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> K </span> </div> )} </span> ); };Then use it in both places:
<div className="absolute top-1/2 right-2 transform -translate-y-1/2 flex items-center space-x-2 text-xs text-gray-500"> - {open ? ( - <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> - ESC - </span> - ) : ( - <span> - {isAppleDevice ? ( - <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> - ⌘K - </span> - ) : ( - <div className="flex gap-1 font-medium"> - <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> - Ctrl - </span> - <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> - K - </span> - </div> - )} - </span> - )} + <KeyboardShortcutHint isOpen={open} /> </div>Also applies to: 226-249
130-140: Enhance keyboard navigation robustness.The keyboard navigation logic could be improved in two areas:
- Add visual feedback when no item is focused (focusedIndex === -1)
- Make the Enter key handler more defensive
Apply these improvements:
} else if (e.key === "Enter" && focusedIndex !== -1) { + if (focusedIndex >= unselectedOptions.length) { + return; + } const selectedOptionIndex = options.findIndex( (option) => option.key === unselectedOptions[focusedIndex].key, ); + if (selectedOptionIndex === -1) { + return; + } handleOptionChange(selectedOptionIndex); }Also, consider adding a CSS class to indicate when no item is focused:
className={cn( "flex items-center p-2 rounded-md cursor-pointer", { "bg-gray-100": focusedIndex === index, + "bg-gray-50": focusedIndex === -1, "hover:bg-secondary-100": true, }, )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Common/SearchByMultipleFields.tsx(5 hunks)src/pages/Encounters/EncounterList.tsx(2 hunks)src/pages/Organization/OrganizationPatients.tsx(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/Encounters/EncounterList.tsx
[error] 165-175: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
src/pages/Organization/OrganizationPatients.tsx
[error] 55-61: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (1)
src/components/Common/SearchByMultipleFields.tsx (1)
118-126: LGTM! Keyboard shortcut implementation follows best practices.The change from "/" to "Cmd/Ctrl + K" aligns with modern web application patterns and provides a more intuitive search experience. The implementation correctly handles both Mac and Windows/Linux modifier keys.
|
👋 Hi, @AdityaJ2305, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
AdityaJ2305
force-pushed
the
serachinput_design_update
branch
from
91e9334 to
3cfb24a
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Common/SearchByMultipleFields.tsx (1)
383-396: Add accessibility attributes to clear search button.The clear search button needs proper accessibility attributes.
<Button variant="ghost" size="sm" + aria-label={t("clear_search")} className="w-full flex items-center justify-center text-muted-foreground" onClick={() => { setSearchValue(""); inputRef.current?.focus(); }} > <CareIcon icon="l-times" className="mr-2 h-4 w-4" /> {t("clear_search")} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/SearchByMultipleFields.tsx(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (6)
src/components/Common/SearchByMultipleFields.tsx (6)
56-79: Well-structured keyboard shortcut component!Good job on:
- Extracting the component outside the main component
- Using semantic
kbdtags- Handling device-specific shortcuts
141-156: Improved keyboard shortcut handling!Good improvements:
- Added
stopPropagation()to prevent browser's search on Ctrl+K- Enhanced Escape key handling to clear search or close menu
210-234: Clean UI implementation with error handling!Good improvements:
- Added error handling for phone input
- Consistent keyboard shortcut hint placement
- Proper error state management
260-354: Excellent search options UI implementation!Great attention to:
- Keyboard navigation hints with Enter key
- Proper ARIA attributes
- Clear visual feedback for focused state
94-102:⚠️ Potential issueAdd bounds checking for initialOptionIndex.
The
initialOptionIndexshould be validated to prevent array out-of-bounds errors.- const [selectedOptionIndex, setSelectedOptionIndex] = useState(initialOptionIndex); + const [selectedOptionIndex, setSelectedOptionIndex] = useState( + initialOptionIndex < 0 || initialOptionIndex >= options.length + ? 0 + : initialOptionIndex + );Likely invalid or redundant comment.
160-170:⚠️ Potential issueAdd empty state handling for keyboard navigation.
The code is vulnerable to runtime errors when
unselectedOptionsis empty.if (open) { + if (unselectedOptions.length === 0) return; if (e.key === "ArrowDown") { setFocusedIndex((prevIndex) => prevIndex === unselectedOptions.length - 1 ? 0 : prevIndex + 1, ); } else if (e.key === "ArrowUp") { setFocusedIndex((prevIndex) => prevIndex === 0 ? unselectedOptions.length - 1 : prevIndex - 1, ); } else if (e.key === "Enter") { + if (focusedIndex < 0 || focusedIndex >= unselectedOptions.length) return; const selectedOptionIndex = options.findIndex( (option) => option.key === unselectedOptions[focusedIndex].key, ); handleOptionChange(selectedOptionIndex); } }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Common/SearchByMultipleFields.tsx (1)
107-107: Use strict equality operator.Replace
==with===for strict type checking:-const isSingleOption = options.length == 1; +const isSingleOption = options.length === 1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/SearchByMultipleFields.tsx(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (4)
src/components/Common/SearchByMultipleFields.tsx (4)
56-84: Well-implemented keyboard shortcut component!The
KeyboardShortcutHintcomponent follows best practices by using semantickbdtags and handling device-specific shortcuts appropriately.
146-161: Improved keyboard shortcut handling!The implementation properly addresses the reported issues:
- Prevents browser's default search behavior with
stopPropagation- Correctly manages input focus
- Clearly separates Escape key behaviors for menu close and search clear
165-175: Add empty state handling for keyboard navigation.The keyboard navigation logic needs guards for empty state:
if (open) { + if (unselectedOptions.length === 0) return; + if (e.key === "ArrowDown") { setFocusedIndex((prevIndex) => prevIndex === unselectedOptions.length - 1 ? 0 : prevIndex + 1, ); } else if (e.key === "ArrowUp") { setFocusedIndex((prevIndex) => prevIndex === 0 ? unselectedOptions.length - 1 : prevIndex - 1, ); } else if (e.key === "Enter") { + if (focusedIndex < 0 || focusedIndex >= unselectedOptions.length) return; const selectedOptionIndex = options.findIndex( (option) => option.key === unselectedOptions[focusedIndex].key, ); handleOptionChange(selectedOptionIndex); } }
388-401: Add accessibility attributes to clear search button.The clear search button should have proper accessibility attributes:
<Button variant="ghost" size="sm" + aria-label={t("clear_search")} className="w-full flex items-center justify-center text-muted-foreground" onClick={() => { setSearchValue(""); inputRef.current?.focus(); }} > <CareIcon icon="l-times" className="mr-2 h-4 w-4" /> {t("clear_search")} </Button>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pages/Organization/OrganizationPatients.tsx (1)
65-72: Remove redundant block statement.The function contains an unnecessary block statement that can be safely removed.
Apply this diff to simplify the code:
const handleFieldChange = () => { - { - updateQuery({ - name: undefined, - phone_number: undefined, - }); - } + updateQuery({ + name: undefined, + phone_number: undefined, + }); };🧰 Tools
🪛 Biome (1.9.4)
[error] 66-71: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
src/pages/Encounters/EncounterList.tsx (1)
163-174: Remove redundant block statement.The function contains an unnecessary block statement that can be safely removed.
Apply this diff to simplify the code:
const handleFieldChange = () => { - { - updateQuery({ - status, - encounter_class: encounterClass, - priority, - name: undefined, - encounter_id: undefined, - external_identifier: undefined, - }); - } + updateQuery({ + status, + encounter_class: encounterClass, + priority, + name: undefined, + encounter_id: undefined, + external_identifier: undefined, + }); };🧰 Tools
🪛 Biome (1.9.4)
[error] 164-173: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
public/locale/en.json(2 hunks)src/components/Facility/CreateFacilityForm.tsx(2 hunks)src/pages/Encounters/EncounterList.tsx(3 hunks)src/pages/Facility/FacilitiesPage.tsx(2 hunks)src/pages/Organization/OrganizationPatients.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/Facility/CreateFacilityForm.tsx
- src/pages/Facility/FacilitiesPage.tsx
- public/locale/en.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/Encounters/EncounterList.tsx
[error] 164-173: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
src/pages/Organization/OrganizationPatients.tsx
[error] 66-71: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
src/pages/Organization/OrganizationPatients.tsx (4)
33-33: LGTM!The cacheBlacklist is correctly updated to include the search fields used in the component.
37-50: LGTM!The searchOptions array is well-structured with clear placeholders and proper typing.
106-112: LGTM!The SearchByMultipleFields component is correctly configured with the new props.
52-63: 🛠️ Refactor suggestionImprove phone number validation logic.
The current validation logic for phone numbers (length >= 13) seems arbitrary and might prevent searching for valid phone numbers with different formats.
Consider using a more flexible validation approach:
- key === "phone_number" - ? value.length >= 13 || value === "" - ? value - : undefined - : undefined, + key === "phone_number" + ? value.trim() || undefined + : undefined,Let the
PhoneNumberFormFieldcomponent handle the validation and error display instead.Likely invalid or redundant comment.
src/pages/Encounters/EncounterList.tsx (3)
151-154: LGTM!The cacheBlacklist is correctly updated to include all the search fields used in the component.
275-279: LGTM!The
onEscapeKeyDownhandler is correctly added to prevent the default Esc key behavior, addressing the issue mentioned in the PR objectives.
287-293: LGTM!The SearchByMultipleFields component is correctly configured with the new props.
|
👋 Hi, @AdityaJ2305, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/components/Facility/FacilityForm.tsx (5)
46-68: Consider making the phone number validation more flexible for international usage.The phone number validation is currently hardcoded for Indian numbers (+91). Consider making it more flexible to support international phone numbers.
- .regex( - /^\+91[0-9]{10}$/, - "Phone number must start with +91 followed by 10 digits", - ), + .regex( + /^\+[1-9]\d{1,14}$/, + "Please enter a valid international phone number (E.164 format)", + ),
109-117: Enhance error handling with type safety.Consider using a typed error interface instead of casting to an unknown structure. This will make the error handling more maintainable and type-safe.
interface FacilityError { errors: { msg: string[]; }; } // Usage in onError onError: (error: Error) => { const errorData = error.cause as FacilityError; // ... rest of the code }
148-148: Consider making the geolocation timeout configurable.The 10-second timeout is hardcoded. Consider making it configurable through props or environment variables for different network conditions.
331-331: Internationalize loading and action text.Consider using translation keys for consistency with the rest of the application:
- "Getting Location..."
- "Get Current Location"
- "Creating Facility..."
- "Create Facility"
- Getting Location... + {t("getting_location")} - Get Current Location + {t("get_current_location")} - Creating Facility... + {t("creating_facility")} - "Create Facility" + {t("create_facility")}Also applies to: 336-336, 446-446, 449-449
160-160: Enhance accessibility for form sections.Add aria-labels to the form sections for better screen reader support.
- <h3 className="text-lg font-medium">Basic Information</h3> + <h3 className="text-lg font-medium" aria-label="Basic Information Section">Basic Information</h3> // Apply similar changes to other section headingsAlso applies to: 255-255, 318-318, 387-387
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(2 hunks)src/components/Facility/FacilityForm.tsx(9 hunks)src/pages/Organization/components/GovtOrganizationSelector.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/Organization/components/GovtOrganizationSelector.tsx
- public/locale/en.json
🧰 Additional context used
📓 Learnings (1)
src/components/Facility/FacilityForm.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9852
File: src/components/Facility/FacilityCreate.tsx:505-509
Timestamp: 2025-01-10T10:19:48.667Z
Learning: The `FacilityCreate` component in `src/components/Facility/FacilityCreate.tsx` is being phased out in favor of `CreateFacilityForm`.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (1)
src/components/Facility/FacilityForm.tsx (1)
414-431: Review the organization selector visibility logic.The organization selector is shown when
organizationIdis not provided, but marked as required. This might lead to confusion. Consider:
- Making the logic more explicit
- Adding a comment explaining the conditional rendering
- Validating the organization selection in the form schema
✅ Verification successful
The organization selector logic is correctly implemented ✓
The current implementation handles two valid scenarios:
- When
organizationIdis provided, it's automatically set asgeo_organization- When
organizationIdis not provided, user must select using the required selectorConsider adding a comment explaining this dual-mode behavior for better maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this component is used without organizationId elsewhere rg "CreateFacilityForm" -A 5 -B 5Length of output: 731
Script:
#!/bin/bash # Check for schema definition and validation rg "const.*Schema.*=.*z\." -A 10 src/components/Facility/FacilityForm.tsx # Look for component usage rg "<CreateFacilityForm" -A 3 -B 3Length of output: 533
Script:
#!/bin/bash # Get complete schema definition rg "facilityFormSchema.*=.*z\.object\({" -A 20 src/components/Facility/FacilityForm.tsx # Look for component usage with different patterns rg -l "FacilityForm" | xargs rg "organizationId"Length of output: 938
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/components/Facility/FacilityForm.tsx (2)
Line range hint
116-134: Simplify form submission logic to handle only creation.Since this component is transitioning to a creation-only form:
- Remove the update-related mutation
- Simplify the
onSubmithandler- Clean up the
isPendingstate handling-const { mutate: updateFacility, isPending: isUpdatePending } = useMutation({ - mutationFn: mutate(routes.updateFacility, { - pathParams: { id: facilityId || "" }, - }), - onSuccess: (_data: FacilityModel) => { - toast.success(t("facility_updated_successfully")); - queryClient.invalidateQueries({ queryKey: ["organizationFacilities"] }); - form.reset(); - onSubmitSuccess?.(); - }, -}); const onSubmit = (data: FacilityFormValues) => { - const requestData = { - ...data, - phone_number: parsePhoneNumber(data.phone_number), - }; - - if (facilityId) { - updateFacility(requestData); - } else { - createFacility(requestData); - } + createFacility({ + ...data, + phone_number: parsePhoneNumber(data.phone_number), + }); };
Line range hint
509-533: Simplify submit button to handle only creation state.The submit button still handles both create and update states. Simplify it to match the component's new purpose.
<Button type="submit" className="w-full" - disabled={facilityId ? isUpdatePending : isPending} - data-cy={facilityId ? "update-facility" : "submit-facility"} + disabled={isPending} + data-cy="submit-facility" > - {facilityId ? ( - isUpdatePending ? ( - <> - <CareIcon - icon="l-spinner" - className="mr-2 h-4 w-4 animate-spin" - /> - {t("updating_facility")} - </> - ) : ( - t("update_facility") - ) - ) : isPending ? ( + {isPending ? ( <> <CareIcon icon="l-spinner" className="mr-2 h-4 w-4 animate-spin" /> {t("creating_facility")} </> ) : ( t("create_facility") )} </Button>
🧹 Nitpick comments (1)
src/components/Facility/FacilityForm.tsx (1)
Line range hint
1-47: Consider implementing keyboard shortcuts using useKeyBoardShortcut hook.As suggested in the PR comments by JavidSumra, consider implementing keyboard shortcuts using the
useKeyBoardShortcuthook for better user experience.import { useEffect, useState } from "react"; +import { useKeyBoardShortcut } from "@/hooks/useKeyBoardShortcut"; import { useForm } from "react-hook-form";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/FacilityForm.tsx(2 hunks)src/pages/Organization/components/GovtOrganizationSelector.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/Organization/components/GovtOrganizationSelector.tsx
🧰 Additional context used
📓 Learnings (1)
src/components/Facility/FacilityForm.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9852
File: src/components/Facility/FacilityCreate.tsx:505-509
Timestamp: 2025-01-10T10:19:48.667Z
Learning: The `FacilityCreate` component in `src/components/Facility/FacilityCreate.tsx` is being phased out in favor of `CreateFacilityForm`.
🔇 Additional comments (1)
src/components/Facility/FacilityForm.tsx (1)
Line range hint
384-393: Update form schema to match the new organization selector implementation.The form schema still uses
geo_organizationwhile the UI has been updated to useGovtOrganizationSelector. This mismatch could lead to validation issues.Let's verify if this change is consistent across the codebase:
rithviknishad
removed
changes required
Deploy-Failed
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
Component Updates
OrganizationSelectorwithGovtOrganizationSelectorin multiple components.Localization