Search UI for discharge patients #9320
Conversation
WalkthroughThe changes in this pull request focus on enhancing the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
🧹 Outside diff range and nitpick comments (1)
src/components/Facility/DischargedPatientsList.tsx (1)
71-104: Consider enhancing phone number validationWhile the search options are well-structured, consider adding proper phone number validation to ensure consistent format across searches.
{ key: "phone_number", label: "Phone Number", type: "phone" as const, placeholder: "Search_by_phone_number", value: qParams.phone_number || "", shortcutKey: "p", + validate: (value: string) => { + const phoneRegex = /^\+91[1-9]\d{9}$/; + return phoneRegex.test(value) ? null : "Invalid phone number format"; + } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/DischargedPatientsList.tsx(6 hunks)
🔇 Additional comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
2-2: LGTM: Import and hook setup is well-structured
The import changes and useFilters hook implementation are properly configured with appropriate caching controls for search parameters.
Also applies to: 42-42, 54-69
272-272: LGTM: Loading state implementation is well-handled
The loading state is properly managed and displayed, providing good user feedback during search operations.
Also applies to: 304-319, 432-435
304-319: Verify security measures for patient data search
Since this component handles sensitive patient data, ensure that:
- The search API endpoints have proper rate limiting
- Input validation is implemented on the server side
- Access controls are in place to restrict unauthorized searches
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
54-69: Consider documenting the cacheBlacklist purposeThe cacheBlacklist configuration is correct for preventing search terms from being cached, but adding a brief comment explaining its purpose would improve maintainability.
} = useFilters({ limit: 12, + // Prevent caching of search field values to ensure fresh searches cacheBlacklist: [ "name", "patient_no",
71-104: Add TypeScript interface for search optionsConsider adding a type definition for the search options to improve type safety and maintainability.
interface SearchOption { key: string; label: string; type: 'phone' | 'text'; placeholder: string; value: string; shortcutKey: string; } const searchOptions: SearchOption[] = [ // ... existing options ];
268-270: Consider handling concurrent searchesTo prevent race conditions with multiple rapid searches, consider implementing request cancellation.
const [abortController, setAbortController] = useState<AbortController | null>(null); const handleSearch = useCallback((key: string, value: string) => { // Cancel previous request abortController?.abort(); const newController = new AbortController(); setAbortController(newController); // ... existing search logic }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/DischargedPatientsList.tsx(6 hunks)
🔇 Additional comments (4)
src/components/Facility/DischargedPatientsList.tsx (4)
2-2: LGTM: Import changes align with new search functionality
The addition of useCallback and SearchByMultipleFields imports support the new smart search UI implementation.
Also applies to: 42-42
106-126: Refactor handleSearch for better maintainability
The current implementation has repeated logic and could be simplified.
302-317: LGTM: UI updates improve search experience
The integration of SearchByMultipleFields and loading indicators enhances the user experience.
430-433: LGTM: Proper loading state management
The query callback correctly updates both the count and loading state.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/DischargedPatientsList.tsx (2)
97-103: Use consistent naming for emergency contact numberThe
keyinsearchOptionsis"emergency_contact_number", but the corresponding query parameter isqParams.emergency_phone_number. For clarity and to avoid confusion, consider usingemergency_phone_numberas the key to match the query parameter name.Apply this diff to update the key in
searchOptions:{ - key: "emergency_contact_number", + key: "emergency_phone_number", label: "Emergency Contact Phone Number", type: "phone" as const, placeholder: "search_by_emergency_phone_number", value: qParams.emergency_phone_number || "", shortcutKey: "e", },
108-109: Avoid magic numbers by defining a constant for phone number lengthThe number
13is used as a hardcoded value to validate phone number lengths. Consider defining a constant (e.g.,PHONE_NUMBER_MIN_LENGTH) to improve code readability and maintainability.Apply this diff to define a constant:
+const PHONE_NUMBER_MIN_LENGTH = 13; const isValidPhoneNumber = (val: string) => - val.length >= 13 || val === ""; + val.length >= PHONE_NUMBER_MIN_LENGTH || val === "";
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
71-104: Consider using i18n for search option labels and placeholdersThe search options configuration is well-structured, but the labels and placeholders should use the translation function for internationalization consistency.
const searchOptions = [ { key: "phone_number", - label: "Phone Number", + label: t("phone_number"), type: "phone" as const, - placeholder: "Search_by_phone_number", + placeholder: t("search_by_phone_number"), value: qParams.phone_number || "", shortcutKey: "p", }, // Apply similar changes to other options... ];
301-316: Enhance accessibility for search interfaceWhile the UI implementation is good, consider adding ARIA labels and roles for better accessibility.
<div className="mt-4 gap-4 lg:gap-16 flex flex-col lg:flex-row lg:items-center"> - <div id="total-patientcount"> + <div id="total-patientcount" role="status" aria-live="polite"> <CountBlock text={t("total_patients")} count={count || 0} loading={isLoading} icon="d-patient" /> </div> <SearchByMultipleFields id="patient-search" + aria-label={t("search_discharged_patients")} options={searchOptions} onSearch={handleSearch} clearSearch={clearSearch} className="w-full" /> </div>
429-432: Add error handling to query callbackConsider handling potential errors in the query callback to provide better user feedback.
queryCB={(query) => { setCount(query.data?.count || 0); setIsLoading(query.loading); + if (query.error) { + // Consider using a toast or error boundary + console.error('Failed to fetch patients:', query.error); + } }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/DischargedPatientsList.tsx(6 hunks)
🔇 Additional comments (4)
src/components/Facility/DischargedPatientsList.tsx (4)
2-2: LGTM: Import statements are appropriate
The added imports support the new search functionality implementation.
Also applies to: 42-42
106-126: Previous refactoring suggestion has been implemented correctly
The handleSearch implementation follows the recommended pattern from the previous review.
269-269: Incomplete useState call needs to be removed
There's an incomplete useState call that needs to be removed.
54-69: Verify cacheBlacklist implementation in useFilters hook
The cacheBlacklist configuration looks correct for preventing search fields from being cached. However, let's verify the implementation in the useFilters hook.
✅ Verification successful
The cacheBlacklist implementation is correctly used in useFilters hook
The implementation properly handles the cacheBlacklist parameter:
- It combines the provided cacheBlacklist with default FILTERS_CACHE_BLACKLIST (["page", "limit", "offset"])
- The combined blacklist is used when setting cache via FiltersCache.set()
- The cache is cleaned using FiltersCache.utils.clean before updating query params
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify useFilters hook implementation
# Look for useFilters hook definition and its cacheBlacklist usage
ast-grep --pattern 'export default function useFilters($_) {
$$$
cacheBlacklist
$$$
}'
Length of output: 94
Script:
#!/bin/bash
# Let's try a broader search for useFilters implementation
rg -l "useFilters" --type ts --type tsx
# Then let's look for files that contain both useFilters and cacheBlacklist
rg "cacheBlacklist.*useFilters|useFilters.*cacheBlacklist" --type ts --type tsx -C 5
Length of output: 182
Script:
#!/bin/bash
# Search for useFilters without file type restrictions
rg -l "useFilters"
# Then search for the implementation with context
rg "export.*function.*useFilters" -C 10
Length of output: 1981
Script:
#!/bin/bash
# Let's see how cacheBlacklist is used in the implementation
rg -A 20 "cacheBlacklist.*=.*\[" src/hooks/useFilters.tsx
Length of output: 652
Script:
#!/bin/bash
# Let's check the FILTERS_CACHE_BLACKLIST constant and FiltersCache implementation
rg "FILTERS_CACHE_BLACKLIST" -B 2 -A 2
rg "FiltersCache" -B 2 -A 5
Length of output: 3149
|
Reordered Discharged patient search buttons as implemented in PR #9319 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/DischargedPatientsList.tsx (2)
71-104: Add translations for search field placeholders.The placeholder values appear to be translation keys (e.g., "search_by_patient_name") but aren't being translated using the t() function.
Apply this diff to add translations:
const searchOptions = [ { key: "name", label: "Name", type: "text" as const, - placeholder: "search_by_patient_name", + placeholder: t("search_by_patient_name"), value: qParams.name || "", shortcutKey: "n", }, { key: "patient_no", label: "IP/OP No", type: "text" as const, - placeholder: "search_by_patient_no", + placeholder: t("search_by_patient_no"), value: qParams.patient_no || "", shortcutKey: "u", }, { key: "phone_number", label: "Phone Number", type: "phone" as const, - placeholder: "Search_by_phone_number", + placeholder: t("search_by_phone_number"), value: qParams.phone_number || "", shortcutKey: "p", }, { key: "emergency_contact_number", label: "Emergency Contact Phone Number", type: "phone" as const, - placeholder: "search_by_emergency_phone_number", + placeholder: t("search_by_emergency_phone_number"), value: qParams.emergency_phone_number || "", shortcutKey: "e", }, ];
106-126: Consider extracting phone number validation to a utility function.The phone number validation logic could be reused across the application.
Consider creating a utility function:
+const isValidPhoneNumber = (val: string) => val.length >= 13 || val === ""; const handleSearch = useCallback( (key: string, value: string) => { - const isValidPhoneNumber = (val: string) => - val.length >= 13 || val === ""; - const updatedQuery = { phone_number: key === "phone_number" && isValidPhoneNumber(value) ? value : undefined, name: key === "name" ? value : undefined, patient_no: key === "patient_no" ? value : undefined, emergency_phone_number: key === "emergency_contact_number" && isValidPhoneNumber(value) ? value : undefined, }; updateQuery(updatedQuery); }, [updateQuery], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/DischargedPatientsList.tsx(6 hunks)
🔇 Additional comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
2-2: LGTM! Hook usage and imports are well-structured.
The changes appropriately set up the component with necessary imports and hook configurations for the new search functionality.
Also applies to: 42-42, 54-69
269-269: LGTM! UI and state management changes are well-implemented.
The changes properly integrate the new search UI with loading states, improving the user experience.
Also applies to: 301-316, 429-432
301-316: Verify the impact on existing search functionality.
The new search UI replaces the old implementation. Let's verify that all existing search scenarios still work as expected.
✅ Verification successful
Search functionality has been properly migrated and enhanced
The new SearchByMultipleFields component maintains all existing search capabilities while adding improvements:
- All previous search parameters (name, patient number, phone, emergency contact) are preserved
- Enhanced input validation for phone numbers
- Improved UX with keyboard shortcuts and type-specific input handling
- Consistent implementation across ManagePatients and DischargedPatientsList
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old search implementation
# and verify that all search parameters are properly handled in the new implementation.
# Check for old search-related code that might need cleanup
rg -l "SearchField|searchField" --type tsx
# Verify that the new SearchByMultipleFields component handles all search parameters
ast-grep --pattern 'SearchByMultipleFields'
Length of output: 942
Script:
#!/bin/bash
# Let's analyze the search implementation in more detail
# Check the implementation of SearchByMultipleFields component
cat src/components/Common/SearchByMultipleFields.tsx
# Check how search is handled in DischargedPatientsList
ast-grep --pattern 'const handleSearch = $_'
# Check the search options setup
ast-grep --pattern 'const searchOptions = $_'
Length of output: 17067
|
Hey @nihal467 can you add needs review and needs testing label for this PR |
|
Hey @Jacobjeevan can you review this PR. Is this relevant PR or should i need to change anything. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/DischargedPatientsList.tsx (2)
70-103: Consider using i18n for search field labels and placeholders.The search configuration is well-structured, but the labels and placeholders should use the translation system for consistency with the rest of the application.
Example changes:
{ key: "name", - label: "Name", + label: t("patient_name_label"), type: "text" as const, - placeholder: "search_by_patient_name", + placeholder: t("search_by_patient_name"), value: qParams.name || "", shortcutKey: "n", },
105-125: Consider extracting phone number validation to a utility function.The phone number validation logic could be reused elsewhere in the application. Consider moving it to a utility function.
Example implementation:
+// In src/Utils/validation.ts +export const isValidPhoneNumber = (value: string): boolean => + value.length >= 13 || value === ""; const handleSearch = useCallback( (key: string, value: string) => { - const isValidPhoneNumber = (val: string) => - val.length >= 13 || val === ""; + import { isValidPhoneNumber } from "@/Utils/validation"; const updatedQuery = { phone_number: key === "phone_number" && isValidPhoneNumber(value)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/DischargedPatientsList.tsx(6 hunks)
🔇 Additional comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
2-2: LGTM! Import changes align with new search functionality.
The addition of useCallback and SearchByMultipleFields imports supports the new smart search implementation.
Also applies to: 14-14
53-68: LGTM! Good implementation of search state management.
The cache blacklist for search fields is a good practice for privacy, and the clearSearch function integration enables proper search state management.
300-315: LGTM! Well-implemented loading state and search UI integration.
The loading state management and search UI integration are properly implemented, providing good user feedback during searches.
Also applies to: 428-431
|
Hey @rithviknishad can you share your review. Let me know if any changes are required. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/components/Facility/DischargedPatientsList.tsx (4)
70-103: Consider adding type safety to search options configuration.While the configuration is well-structured, it could benefit from TypeScript interfaces to ensure type safety and maintainability.
Consider adding these types:
type SearchOptionType = 'text' | 'phone'; interface SearchOption { key: string; label: string; type: SearchOptionType; placeholder: string; value: string; shortcutKey: string; } const searchOptions: SearchOption[] = [ // ... existing configuration ];
105-125: Consider enhancing phone number validation.The current validation only checks the length of the phone number. Consider adding a more robust validation pattern.
Consider this enhancement:
- const isValidPhoneNumber = (val: string) => - val.length >= 13 || val === ""; + const isValidPhoneNumber = (val: string) => { + if (val === "") return true; + const phoneRegex = /^\+\d{1,3}\d{10}$/; + return phoneRegex.test(val); + };
268-268: Consider optimizing loading state management.The current implementation maintains a separate loading state, but you could directly use the loading state from the query to avoid unnecessary state updates.
Consider this optimization:
- const [isLoading, setIsLoading] = useState(false); + const [count, setCount] = useState(0); return ( // ... <CountBlock text={t("total_patients")} count={count || 0} - loading={isLoading} + loading={query.loading} /> // ... <PaginatedList queryCB={(query) => { setCount(query.data?.count || 0); - setIsLoading(query.loading); }} />Also applies to: 300-315, 428-431
509-510: Remove unnecessary empty lines and comments at the end of the file.The empty lines and the single-line comment
//at the end of the file should be removed.- -//
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/DischargedPatientsList.tsx(7 hunks)
🔇 Additional comments (1)
src/components/Facility/DischargedPatientsList.tsx (1)
Line range hint 1-68: LGTM! Import statements and hook configuration look good.
The imports are using absolute paths and the useFilters hook is properly configured with appropriate cache blacklist entries for search fields.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
70-125: Consider enhancing maintainability and internationalization.While the search implementation is solid, consider these improvements:
- Extract the phone number length validation (13) into a constant
- Move the search option labels to i18n files for better internationalization support
Example implementation:
+ const PHONE_NUMBER_MIN_LENGTH = 13; + const searchOptions = [ { - label: "Name", + label: t("patient_search.name_label"), // ... other properties }, // ... other options ]; const handleSearch = useCallback( (key: string, value: string) => { const isValidPhoneNumber = (val: string) => - val.length >= 13 || val === ""; + val.length >= PHONE_NUMBER_MIN_LENGTH || val === "";
268-268: Consider semantic loading state initialization.The loading state implementation is good, but consider initializing it to
truesince data is being fetched on component mount:- const [isLoading, setIsLoading] = useState(false); + const [isLoading, setIsLoading] = useState(true);This would prevent a flash of "0" in the CountBlock before the actual count is loaded.
Also applies to: 300-315
428-431: Consider using object destructuring for cleaner code.The queryCB implementation could be more concise:
- queryCB={(query) => { - setCount(query.data?.count || 0); - setIsLoading(query.loading); - }} + queryCB={({ data, loading }) => { + setCount(data?.count || 0); + setIsLoading(loading); + }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/DischargedPatientsList.tsx(6 hunks)
🔇 Additional comments (1)
src/components/Facility/DischargedPatientsList.tsx (1)
2-2: LGTM! Well-structured hook usage and imports.
The cache blacklist configuration correctly prevents persisting search-related fields, which is the desired behavior for search functionality.
Also applies to: 14-14, 53-68
…/Mahendar0701/care_fe into search-ui-for-discharge-patients
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
SearchByMultipleFieldscomponent.Bug Fixes
Improvements