Fix state & district autofill & multiple other small issues in Patient Registration page #9711
Conversation
WalkthroughThe pull request introduces improvements to the patient registration workflow, focusing on enhancing pincode-based auto-filling of state and district information. The changes modify the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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. |
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
yash-learner
changed the title
yash-learner
changed the title
yash-learner
changed the title
| async function fetchOrganizationByName(name: string, parentId?: string) { | ||
| try { | ||
| const data = await query(organizationApi.list, { | ||
| queryParams: { | ||
| org_type: "govt", | ||
| parent: parentId || "", | ||
| name, | ||
| }, | ||
| })({ signal: new AbortController().signal }); | ||
| return data.results?.[0]; | ||
| } catch (error) { | ||
| toast.error("Error fetching organization"); | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
At some point, we may need to extract this into a separate hook
There was a problem hiding this comment.
Yea, go ahead and do that since same logic is being used in #9662.
Also check Rithvik's comment below.
cc: @Mahendar0701
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/pages/Organization/components/OrganizationSelector.tsx (2)
23-24: Add documentation for new props.
Prefer adding a short explanation or a JSDoc comment forparentSelectedLevelsanderrorMessageto guide future maintainers on their usage.
170-171: Push only meaningful errors.
errors={[props.errorMessage || ""]}might cause an empty string to appear if there's no actual error. Consider conditionally passing an empty array instead to avoid confusing UI.-errors={[props.errorMessage || ""]} +errors={props.errorMessage ? [props.errorMessage] : []}src/components/Patient/PatientRegistration.tsx (4)
6-6: Confirm i18n usage for toast messages.
Since you are using translations in other notifications, consider localizing toast errors to maintain a consistent user experience.
208-213: Consider grouping pincode org fetch logic.
The logic fetches the state org first, then setsselectedLevelsto[]early if state org is not found. Consider returning a partial selection if the state is found but not the district – or add a fallback mechanism.
326-340: Add robust error details.
toast.error("Error fetching organization")might leave users uncertain about the exact cause. Consider including the organization name in the error text or using a more descriptive message.
625-625: Improve UX for disabled text area.
You correctly remove editing withcursor-not-allowedif addresses match. Optionally, add a tooltip or note explaining why the field is disabled for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Patient/PatientRegistration.tsx(7 hunks)src/pages/Organization/components/OrganizationSelector.tsx(5 hunks)
🔇 Additional comments (6)
src/pages/Organization/components/OrganizationSelector.tsx (3)
33-36: Initialize state thoughtfully.
UsingparentSelectedLevels || []is fine. Consider logging or handling scenarios whereparentSelectedLevelsmight be empty due to unexpected data, ensuring robust default behaviors.
112-124: Validate data removal edge cases.
When the user removes all levels (newLevels.length === 0), you reset the selection. This is correct for typical usage. Confirm that removing the top-level organization doesn't break parent-child flows if the parent-level has siblings or alternative branches.
127-132: Avoid re-initialization loops.
TheuseEffectreassigns state whenparentSelectedLevelschanges. This is correct but be aware of potential repeated updates if parent-level changes are frequent.src/components/Patient/PatientRegistration.tsx (3)
85-85: Validate usage of local org state.
selectedLevelsis used to maintain hierarchical organization data derived from the pincode. Ensure that clearing it in one place does not inadvertently disrupt other parts of registration.
219-222: Handle partial fetch failures.
If the district API fails, current logic omits selecting the state org. Evaluate whether you want to preserve the state org even if the district is not found.
676-683: Check null-safety for error message.
You passerrorMessage={errors.geo_organization?.[0]}but ifgeo_organizationis missing or empty, it may show an undefined or empty string inOrganizationSelector. Confirm the UI reflects a real error only when relevant.
There was a problem hiding this comment.
not a fan of this approach. how about we stick with how dependent queries are made as per tanstacks docs?
this way we wouldn't need to try catch either and stick with the simplicity.
cc @bodhish
|
👋 Hi, @yash-learner, 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 (12)
src/hooks/useOrganization.ts (2)
7-12: Add User Facing Documentation Comment forUseOrganizationParamsThough this interface is self-explanatory, adding a short JSDoc comment or code comment could clarify the accepted parameters for future maintainers and reduce confusion about which fields are optional.
20-31: Ensure error handling is robustWhile
isErroris returned, there is no specialized error handling in thequeryFnor anonErrorcallback for the query. If additional failover or fallback logic is required when the API call fails, consider adding a customonErrorin theuseQueryoptions or a shared error boundary.src/hooks/useStateAndDistrictFromPincode.ts (3)
14-16: Clarify hook naming or add doc commentThe function name
useStateAndDistrictFromPincodeis descriptive, but consider adding inline documentation about how it fetches pincode details, extracts the state and district, and loads the corresponding organizations. This can improve maintainability.
43-53: Consider concurrency or caching benefitsThe second organization query is nested behind the first's result for
stateOrg?.id. This is logical. If you anticipate repeated lookups or the same pincodes, ensure that React Query’s caching strategy is adequately configured to minimize network calls.
56-61: CombineisErrorstates carefullyThis hook merges three possible error states into one, which is correct for a simplified approach. However, distinct error messages can help users identify which step failed (e.g., the pincode fetch vs. the state org fetch). Consider returning more granular error states if needed.
src/components/Patient/PatientRegistration.tsx (7)
6-6: Check the usage ofCareIconimportThe
CareIconimport is newly added. Verify whether all necessary icons are being efficiently imported. For performance, you might consider a dynamic icon library approach if your codebase loads many icons.
30-30: Mention the custom hook in documentation or inline comments
useStateAndDistrictFromPincodeis introduced here. A short comment indicating its purpose in the patient registration flow (e.g., “Auto-fills state and district based on pincode”) could guide future contributors.
79-79: Refactor state management for clarity
selectedLevelsandsetSelectedLevelshold the acquired organizations. Consider whether these should be combined or abstracted to a single object (likegeoOrganization) if there’s no plan to scale beyond two levels. This might simplify future expansions.
189-191: Decouple logic from the UI effectThe
useStateAndDistrictFromPincodehook is invoked inline here. If you anticipate using the same logic in other forms, it might be beneficial to centralize or abstract the effect of pincode changes, e.g., in a custom reducer or separate function for clarity.
194-206: Enhance user feedback for partial auto-fillIf some part of the data can’t be automatically derived from the pincode (e.g., the district wasn’t found), the
pincode_autofillmessage still appears iflevels.length > 0. Consider either partial messages or fallback for missing details.
580-580: UI clarity for disabled text areaWhen
sameAddressis true, the permanent address is disabled. Thecursor-not-allowedis good, but consider adding a tooltip or helper text to clarify that the permanent address matches the current address. This could improve UX.
Line range hint
596-606: Refine timed feedback approachYou’re displaying
_showAutoFilledPincodefor 2 seconds to highlight auto-filling. This is a subtle user experience detail. If users need more time or have accessibility needs, consider a more stable or dismissible notification.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Patient/PatientRegistration.tsx(8 hunks)src/hooks/useOrganization.ts(1 hunks)src/hooks/useStateAndDistrictFromPincode.ts(1 hunks)
🔇 Additional comments (5)
src/hooks/useOrganization.ts (1)
14-19: Confirm default parameter usage logicCurrently, the default values for
orgType,parentId, andnameare all empty strings. If the intended scenario requires no query to run until a validnameis provided, this setup is fine. Otherwise, you might consider enabling queries based on other parameter conditions (likeorgType).src/hooks/useStateAndDistrictFromPincode.ts (2)
17-25: Validate pincode more thoroughly
validatePincode(pincode)is used to control theenabledproperty, but ensure it returnsfalsefor invalid or partial input. If user input can be partially typed, consider whether the query should wait until the pincode is a specific length (e.g., 6 digits in India).
30-40: Check for fallback behavior whenstateNameis missingIf
pincodeDetails.statenameis an empty string, the state query is disabled, which is correct. Just confirm that this is the desired behavior. If there is a possibility of partial matches or alternate naming, you might need more robust fallback or alias logic forstateName.src/components/Patient/PatientRegistration.tsx (2)
43-46: Validate usage of new or changed importsWith the introduction of
OrganizationSelectorfrom@/pages/Organization/components/OrganizationSelector, ensure no cyclical imports exist and confirm version compatibility. This is more of a housekeeping check.
631-638: Verify theOrganizationSelectorusage
OrganizationSelectoris receivingparentSelectedLevelsand anerrorMessage. Ensure that the error logic is consistent with your form validation approach. If you have an external form library or inline validation, confirm that the error message updates as expected.
Co-authored-by: Jacob John Jeevan <mail@jacobjeevan.me> Co-authored-by: mahendar <mahendarchikkolla@gmail.com>
Jacobjeevan
force-pushed
the
issues/9668/fix-auto-fill
branch
from
2ac7d08 to
1d3740e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Patient/PatientRegistration.tsx (2)
198-218: Consider optimizing the useEffect cleanup.The effect handles the autofill notification well, but there are a few improvements possible:
- The cleanup function could be more explicit about its purpose
- The effect could be split into two separate effects - one for levels management and one for notification
- useEffect(() => { - if (patientId) return; - const levels: Organization[] = []; - if (stateOrg) levels.push(stateOrg); - if (districtOrg) levels.push(districtOrg); - setSelectedLevels(levels); - - if (levels.length == 2) { - setShowAutoFilledPincode(true); - const timer = setTimeout(() => { - setShowAutoFilledPincode(false); - }, 5000); - return () => clearTimeout(timer); - } - return () => setShowAutoFilledPincode(false); - }, [stateOrg, districtOrg, patientId]); + useEffect(() => { + if (patientId) return; + const levels: Organization[] = []; + if (stateOrg) levels.push(stateOrg); + if (districtOrg) levels.push(districtOrg); + setSelectedLevels(levels); + }, [stateOrg, districtOrg, patientId]); + + useEffect(() => { + if (patientId) return; + if (stateOrg && districtOrg) { + setShowAutoFilledPincode(true); + const timer = setTimeout(() => { + setShowAutoFilledPincode(false); + }, 5000); + return () => { + clearTimeout(timer); + setShowAutoFilledPincode(false); + }; + } + }, [stateOrg, districtOrg, patientId]);
Line range hint
277-293: Simplify the form reset logic.The current implementation has nested ternary operations which could be simplified for better readability.
- setSelectedLevels([ - patientQuery.data.geo_organization as unknown as Organization, - ]); - form.reset({ - ...patientQuery.data, - same_phone_number: - patientQuery.data.phone_number === - patientQuery.data.emergency_phone_number, - same_address: - patientQuery.data.address === patientQuery.data.permanent_address, - age_or_dob: patientQuery.data.date_of_birth ? "dob" : "age", - age: !patientQuery.data.date_of_birth - ? patientQuery.data.age - : undefined, - date_of_birth: patientQuery.data.date_of_birth - ? patientQuery.data.date_of_birth - : undefined, + const { date_of_birth, age, phone_number, emergency_phone_number, address, permanent_address, geo_organization } = patientQuery.data; + setSelectedLevels([geo_organization as unknown as Organization]); + + form.reset({ + ...patientQuery.data, + same_phone_number: phone_number === emergency_phone_number, + same_address: address === permanent_address, + age_or_dob: date_of_birth ? "dob" : "age", + age: date_of_birth ? undefined : age, + date_of_birth: date_of_birth || undefined,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Patient/PatientRegistration.tsx(10 hunks)
🔇 Additional comments (3)
src/components/Patient/PatientRegistration.tsx (3)
84-85: LGTM! State management is well-structured.The new state variables are appropriately typed and follow React's best practices.
706-721: LGTM! Accessibility implementation is well done.The autofill notification includes proper ARIA attributes and follows accessibility best practices:
- Uses
role="status"for live regions- Includes
aria-live="polite"for screen readers- Marks icon as
aria-hidden="true"
761-764: LGTM! Organization selector integration is correct.The component properly handles the selected levels and value propagation.
| @@ -757,7 +794,7 @@ export default function PatientRegistration( | |||
| </div> | |||
| {!patientPhoneSearch.isLoading && | |||
| !!duplicatePatients?.length && | |||
| !!parsePhoneNumber(debouncedNumber || "") && | |||
| !!parsePhoneNumber(form.watch("phone_number") || "") && | |||
There was a problem hiding this comment.
💡 Codebase verification
Phone number parsing implementation needs improvement
The current implementation has several limitations that could affect robustness:
- The minimum length check of 12 digits is too restrictive for valid international numbers
- Special case handling of "+91" should be generalized
- Limited validation of number format and country codes
🔗 Analysis chain
Verify phone number parsing edge cases.
The phone number parsing should be tested with various formats to ensure robust handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for phone number parsing usage and validation
rg -A 2 "parsePhoneNumber"
Length of output: 4594
Script:
#!/bin/bash
# Get the implementation of parsePhoneNumber function
ast-grep --pattern 'export const parsePhoneNumber = $_'
Length of output: 944
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/hooks/useStateAndDistrictFromPincode.ts (5)
1-1: Consider moving API key to environment variables.The API key should not be directly exposed in the configuration file. Consider moving it to environment variables for better security.
Also applies to: 30-30
28-32: Enhance query configuration and type safety.Consider the following improvements:
- Add retry configuration for better error handling
- Configure stale time for caching optimization
- Specify error type for better type safety
} = useQuery<PincodeResponse>({ queryKey: ["pincode-details", pincode], queryFn: () => getPincodeDetails(pincode, careConfig.govDataApiKey), enabled: pincode !== "" && validatePincode(pincode), + retry: 2, + staleTime: 5 * 60 * 1000, // 5 minutes + gcTime: 10 * 60 * 1000, // 10 minutes });
37-49: Improve state organization selection logic.The current implementation blindly selects the first organization without validation. Consider adding validation and using select transform for better performance.
} = useOrganization({ orgType: "govt", parentId: "", name: stateName, enabled: !!stateName, + select: (orgs) => { + const validOrg = orgs.find(org => + org.type === "State" && org.name.toLowerCase() === stateName?.toLowerCase() + ); + return validOrg ? [validOrg] : []; + } }); - const stateOrg = stateOrgs?.[0]; + const stateOrg = stateOrgs?.[0] ?? null;
50-60: Apply similar improvements to district organization query.Consider applying the same improvements as suggested for the state organization query.
} = useOrganization({ orgType: "govt", parentId: stateOrg?.id, name: districtName, enabled: !!stateOrg?.id && !!districtName, + select: (orgs) => { + const validOrg = orgs.find(org => + org.type === "District" && org.name.toLowerCase() === districtName?.toLowerCase() + ); + return validOrg ? [validOrg] : []; + } });
61-66: Enhance error handling with specific error cases.The current error handling is basic. Consider:
- Adding specific error cases (network, validation, not found)
- Providing more detailed feedback to users
- Adding error boundaries for unexpected failures
- if (isStateError || isPincodeError) { + if (isPincodeError) { + toast.error(t("pincode_invalid_or_not_found")); + } else if (isStateError) { + toast.error(t("state_org_fetch_failed")); + } - if (isDistrictError && !isStateError) { + if (isDistrictError && stateOrg) { toast.info(t("pincode_district_auto_fill_error")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/hooks/useOrganization.ts(1 hunks)src/hooks/useStateAndDistrictFromPincode.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/useOrganization.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: CodeQL-Build
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
|
👋 Hi, @yash-learner, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
LGTM |
|
@yash-learner Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
auto_fill_state_._district_based_on_pincode.webm
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
Test Updates
The changes focus on improving the patient registration experience by introducing more intelligent location-based data entry and simplifying the user interface.