Fix : Pincode exception error and auto populate of geo_organization info on facility edit #9662
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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. |
|
On facility edit the organisation is also not getting prefilled, I think we should solve that also |
You mean |
|
I don't think jeevan is working on it, I will assign it to you. lets have one PR. |
![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 (1)
src/pages/Organization/components/OrganizationSelector.tsx (1)
35-55: Enhance defensive checks around JSON parsing.
Currently, atry/catchblock is in place to catchJSON.parseerrors, logging them to the console. Consider adding a fallback path or early return to ensure the component’s state remains stable in edge cases (e.g., an empty string or invalid structure) to avoid undesired behavior.useEffect(() => { if (value) { try { const parsedValue = typeof value === "string" ? JSON.parse(value) : value; if (parsedValue) { // ... } } catch (e) { + setSelectedLevels([]); console.error("Invalid value for geo_organization:", e); } } }, [value]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/components/Facility/FacilityCreate.tsx(4 hunks)src/pages/Organization/components/OrganizationSelector.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Facility/FacilityCreate.tsx
🔇 Additional comments (4)
src/pages/Organization/components/OrganizationSelector.tsx (3)
2-2: Import statement is correctly updated.
No issues found with the import of React hooks.
31-31: Prop destructuring for newvalueis fine.
Thevalueprop is properly introduced here, and the necessary props are being destructured clearly.
171-171: No issues with the Autocomplete value assignment.
Using the last selected organization’sidor an empty string is a sensible approach.public/locale/en.json (1)
1584-1584: New i18n key added.
The"select_location_from": "Select location from"entry aligns with the updated UI text references. Ensure that its usage is consistent across the app, and consider whether this prompt needs additional context or placeholders.
Mahendar0701
changed the title
|
the issue still presist, can you attach a video after fixing the issue |
Mahendar0701
changed the title
|
edit may work fine, but is it auto-filling when pincode is entered? |
Nope , i will fix that too 👍 |
src/Utils/utils.ts
Outdated
| export function useFetchOrganizationByName(name: string, parentId?: string) { | ||
| return useQuery({ | ||
| queryKey: ["organization", name, parentId], | ||
| queryFn: async () => { | ||
| const data = await query(organizationApi.list, { | ||
| queryParams: { | ||
| org_type: "govt", | ||
| parent: parentId || "", | ||
| name: name || "", | ||
| }, | ||
| })({ signal: new AbortController().signal }); | ||
|
|
||
| return data.results?.[0]; | ||
| }, | ||
| enabled: !!name, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
@bodhish @rithviknishad @Jacobjeevan you mean something like this?? #9711 (review)
There was a problem hiding this comment.
move this to types/organization/organizationApi itself instead of defining it here.
| const { data: pincodeData, isError: isPincodeError } = useQuery({ | ||
| queryKey: ["pincodeDetails", pincode], | ||
| queryFn: () => getPincodeDetails(pincode, careConfig.govDataApiKey), | ||
| enabled: validatePincode(pincode), | ||
| }); | ||
|
|
||
| if (isPincodeError) { | ||
| Notification.Error({ msg: "Invalid pincode" }); | ||
| } | ||
|
|
||
| const stateName = pincodeData?.statename; | ||
|
|
||
| const districtName = pincodeData?.districtname; | ||
|
|
||
| const { data: stateOrg } = useFetchOrganizationByName(stateName); | ||
| const { data: districtOrg } = useFetchOrganizationByName( | ||
| districtName, | ||
| stateOrg?.id, | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (stateOrg && districtOrg) { | ||
| setSelectedLevels([stateOrg, districtOrg]); | ||
| } else { | ||
| setSelectedLevels([]); | ||
| } | ||
| }, [stateOrg, districtOrg]); | ||
|
|
||
| console.log("selectedLevels", selectedLevels); | ||
|
|
||
| const handlePincodeChange = (value: string) => { | ||
| setPincode(value); | ||
| }; | ||
|
|
There was a problem hiding this comment.
used usequery tanstack, is this approach acceptable??
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/Utils/utils.ts (1)
195-198: Clarify the fallback behavior after invalid pincodes
When no records are returned, the function simply returnsnullafter displaying an error notification. This behavior might cause unexpected downstream null reference checks. Consider returning an empty object or a more descriptive error object to better inform consuming code.if (!data.records || data.records.length === 0) { Notification.Error({ msg: "Invalid pincode" }); - return null; + return { error: true }; }src/components/Facility/FacilityCreate.tsx (3)
62-66: Group relevant imports and reduce overhead
Multiple utility imports are introduced in sequential lines. Consider grouping them to maintain concise import sections if your coding guidelines permit it.
238-243: Check for concurrency issues in organization data
When bothstateOrganddistrictOrgare queried, ifstateOrgchanges faster, you might fetch an outdateddistrictOrg. Consider a single combined query for more reliable data.
244-251: Improve readability when clearing selected levels
Ensure the user experience is clear if no valid state/district org is found. Potential default fallback or user-facing notice might be beneficial.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/Utils/utils.ts(3 hunks)src/components/Facility/FacilityCreate.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🔇 Additional comments (6)
src/Utils/utils.ts (1)
202-219: Ensure consistent error handling and loading states in consumer code
This hook returns the first fetched organization, but the consumer code must handle errors and loading states. Double-check if auto-retries or stale data could cause unexpected behavior.✅ Verification successful
Let me gather more context about the error handling in the consumer code.
Error handling and loading states are properly implemented in consumer code
The consumer code in
FacilityCreate.tsxproperly handles both error and loading states:
- The hook is used with proper destructuring of
dataonly:const { data: stateOrg } = useFetchOrganizationByName(stateName)- Loading states are handled through a global
isLoadingstate that's used in the submit button- The hook is enabled conditionally based on the presence of
nameparameter (as seen in the hook implementation)- The component handles missing data gracefully through optional chaining when using
stateOrg?.id- The results are used in a useEffect that safely handles both presence and absence of data:
if (stateOrg && districtOrg) { ... } else { ... }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for usage of `useFetchOrganizationByName` to verify error handling rg -A 3 "useFetchOrganizationByName"Length of output: 1056
Script:
#!/bin/bash # Check the implementation of FacilityCreate.tsx to see how errors and loading states are handled cat src/components/Facility/FacilityCreate.tsxLength of output: 18685
src/components/Facility/FacilityCreate.tsx (5)
77-79: Pincode state management works but consider form synchronization
Defining a separatepincodestate is helpful, but be mindful of potential duplication with the React Hook Form field. Ensure they remain in sync.
234-237: Gracefully handle missing pincode data
Ensurestatenameordistrictnameare not undefined before using them downstream.
364-364: Validate changed pincode
Every time the user updates pincode, ensure it’s validated. The existing approach is correct, but confirm that a partial pincode doesn’t trigger spurious queries.
376-381: Double-check geo_organization updates
When updating thegeo_organizationfield, verify thatOrganizationSelectoris providing correct data. Also ensure any default or fallback organization is properly managed if none is selected.
463-463: Good usage of translation function
The introduced translation for "Select location from" is consistent with i18n best practices.
|
👋 Hi, @Mahendar0701, 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: 1
🧹 Nitpick comments (2)
src/pages/Organization/components/OrganizationSelector.tsx (2)
170-170: Enhance type safety for Autocomplete valueConsider adding type assertion for better type safety.
- value={selectedLevels[selectedLevels.length - 1]?.id || ""} + value={(selectedLevels[selectedLevels.length - 1]?.id as string) || ""}
31-31: Consider splitting component responsibilitiesThe TODO comment suggests renaming to
GovtOrganizationSelector, but there are broader architectural improvements possible:
- Split the display of selected organizations and the selection interface into separate components
- Create a custom hook for organization data fetching logic
- Consider implementing a proper state machine for the multi-level selection process
This would improve maintainability and make the component more focused on its core responsibilities.
Would you like me to provide a detailed proposal for this refactoring?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(1 hunks)src/pages/Organization/components/OrganizationSelector.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🧰 Additional context used
🪛 eslint
src/pages/Organization/components/OrganizationSelector.tsx
[error] 2-3: Delete ⏎
(prettier/prettier)
🪛 GitHub Check: lint
src/pages/Organization/components/OrganizationSelector.tsx
[failure] 2-2:
Delete ⏎
🪛 GitHub Actions: Lint Code Base
src/pages/Organization/components/OrganizationSelector.tsx
[error] 2-2: Prettier formatting error: Delete unnecessary line break
🔇 Additional comments (2)
src/pages/Organization/components/OrganizationSelector.tsx (2)
23-23: LGTM: Props interface enhancementThe addition of
parentSelectedLevelsprop and its proper destructuring aligns well with the requirement to support auto-population of organization information.Also applies to: 33-33
37-54: Verify integration with facility edit formWhile the changes support organization auto-population, we should verify the integration with the facility edit form:
- Confirm that the parent component correctly passes the organization hierarchy
- Ensure the component handles facility edit state transitions properly
Let's verify the integration:
✅ Verification successful
Let me gather more information about the integration:
Integration with facility edit form is properly implemented
The verification shows that the OrganizationSelector integration is correctly implemented:
In FacilityCreate.tsx:
- Maintains organization hierarchy state via
selectedLevels- Properly passes
parentSelectedLevelsto OrganizationSelector- Updates form state via
form.setValue("geo_organization", value)In CreateFacilityForm.tsx:
- Uses OrganizationSelector within a form field
- Handles value changes through form control
The component correctly handles both create and edit scenarios, with proper state management and form integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for FacilityCreate/Edit components that use OrganizationSelector rg -l "OrganizationSelector" src/components/Facility/ # Check how parentSelectedLevels is populated rg -A 5 "parentSelectedLevels" src/components/Facility/Length of output: 717
Script:
#!/bin/bash # Check how selectedLevels is managed in FacilityCreate ast-grep --pattern 'const selectedLevels = $_' src/components/Facility/FacilityCreate.tsx # Check the form initialization and state management rg -B 5 -A 10 "selectedLevels" src/components/Facility/FacilityCreate.tsx # Check CreateFacilityForm usage of OrganizationSelector rg -B 5 -A 10 "OrganizationSelector" src/components/Facility/CreateFacilityForm.tsxLength of output: 3122
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Facility/FacilityCreate.tsx (1)
Add unit tests for the FacilityCreate component
Based on the verification results, there appears to be no existing test coverage for the FacilityCreate component or any other React components in the codebase. While there is a basic test script in package.json, it's only configured for Snyk security testing. The component handles complex form logic including validation, API integration, and error handling, making it a critical candidate for testing.
Key areas to cover in tests:
- Form field validation logic
- Pincode lookup functionality
- Organization selection behavior
- API integration for facility creation
- Error handling scenarios
- Loading state management
Consider setting up a testing framework like Jest with React Testing Library to implement these tests.
🔗 Analysis chain
Line range hint
74-77: Add unit tests for the FacilityCreate componentConsider adding unit tests to cover:
- Form validation
- Pincode lookup and organization selection
- Error handling scenarios
- Integration with the backend API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing test files fd -e test.tsx -e spec.tsx "FacilityCreate"Length of output: 43
Script:
#!/bin/bash # Let's try a broader search for test files in the repository fd -e test.tsx -e spec.tsx # Also search for any test files containing "Facility" in case the naming is different fd -e test.tsx -e spec.tsx "Facility" # Let's also check if there's a __tests__ directory structure fd -t d "__tests__" # Search for any test-related imports or test setup in the codebase rg -i "import.*test" "src/"Length of output: 236
Script:
#!/bin/bash # Let's check if there's any testing framework setup or configuration files fd "jest.config" "test.config" # Check for any testing utilities or test setup files rg -l "test-utils" "src/" # Look for any testing-related dependencies in package.json cat package.json | grep -i "test" # Check if there are any test files with .test.js/.test.ts extensions fd -e test.js -e test.tsLength of output: 311
♻️ Duplicate comments (1)
src/components/Facility/FacilityCreate.tsx (1)
228-237: 🛠️ Refactor suggestionEnhance error handling and prevent race conditions
While the pincode query implementation is functional, consider these improvements:
- Clear any previously fetched data when a pincode error occurs
- Add debouncing to prevent unnecessary API calls during rapid typing
+ const [debouncedPincode] = useDebounce(pincode, 500); const { data: pincodeData, isError: isPincodeError } = useQuery({ - queryKey: ["pincodeDetails", pincode], + queryKey: ["pincodeDetails", debouncedPincode], - queryFn: () => getPincodeDetails(pincode, careConfig.govDataApiKey), + queryFn: () => getPincodeDetails(debouncedPincode, careConfig.govDataApiKey), - enabled: validatePincode(pincode), + enabled: validatePincode(debouncedPincode), }); if (isPincodeError) { Notification.Error({ msg: "Invalid pincode" }); + // Clear any previously fetched data + form.setValue("geo_organization", ""); + setSelectedLevels([]); }
🧹 Nitpick comments (4)
src/components/Facility/FacilityCreate.tsx (4)
238-257: Optimize organization data fetching and remove debug logging
- The current implementation could cause unnecessary re-renders. Consider using
useMemofor derived state.- Remove the debug console.log statement from production code.
- const stateName = pincodeData?.statename; - const districtName = pincodeData?.districtname; + const { stateName, districtName } = useMemo(() => ({ + stateName: pincodeData?.statename, + districtName: pincodeData?.districtname + }), [pincodeData]); const { data: stateOrg } = useFetchOrganizationByName(stateName); const { data: districtOrg } = useFetchOrganizationByName( districtName, stateOrg?.id, ); useEffect(() => { if (stateOrg && districtOrg) { setSelectedLevels([stateOrg, districtOrg]); } else { setSelectedLevels([]); } }, [stateOrg, districtOrg]); - console.log("selectedLevels", selectedLevels);
258-261: Add input sanitization to pincode handlerConsider sanitizing the pincode input to ensure only numeric values are processed.
const handlePincodeChange = (value: string) => { - setPincode(value); + const sanitizedValue = value.replace(/[^0-9]/g, ''); + setPincode(sanitizedValue); };
380-385: Add error handling for organization selectionConsider handling edge cases where the organization selection fails or becomes invalid.
value={facilityData?.geo_organization} parentSelectedLevels={selectedLevels} onChange={(value) => { + try { // Update geo_organization value in form form.setValue("geo_organization", value); + } catch (error) { + console.error('Failed to update organization:', error); + Notification.Error({ msg: t("failed_to_update_organization") }); + } }}
Line range hint
84-117: Improve validation schema maintainabilityConsider extracting validation schema to a separate file for better maintainability and reusability.
+ // src/schemas/facility.ts + export const facilityFormSchema = z.object({...}) + export type FacilityFormValues = z.infer<typeof facilityFormSchema>; - const facilityFormSchema = z.object({...}) - type FacilityFormValues = z.infer<typeof facilityFormSchema>; + import { facilityFormSchema, FacilityFormValues } from '@/schemas/facility';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/components/Facility/FacilityCreate.tsx(7 hunks)src/pages/Organization/components/OrganizationSelector.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- public/locale/en.json
- src/pages/Organization/components/OrganizationSelector.tsx
🔇 Additional comments (2)
src/components/Facility/FacilityCreate.tsx (2)
1-1: LGTM: Clean implementation of new imports and state managementThe new imports and state variables are well-structured and properly typed for managing pincode and organization data.
Also applies to: 63-67, 78-79
264-264: LGTM: Good accessibility and internationalization implementationThe form implementation follows accessibility best practices with proper labels, error messages, and internationalization support.
Also applies to: 467-467
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/components/Facility/FacilityCreate.tsx (6)
225-229: Add user feedback for ongoing requests.
While theuseQueryforpincodeDatais running, the user might not see any indication of loading. Consider adding a loading state or spinner to improve UX.
231-233: Duplicate error notifications.
If multiple invalid pincode entries occur, multiple toasts or notifications might pile up, potentially oversaturating the user. Consider debouncing or grouping errors to reduce user alert fatigue.
245-251: Avoid re-renders when partial data is fetched.
The current logic clearsselectedLevelsifstateOrgordistrictOrgis missing—potentially causing flickers for the user while waiting on the second fetch. Consider a short-circuit if either is still loading.
253-254: Remove debug logs in production.
console.log("selectedLevels", selectedLevels);should likely be removed or wrapped in a debug flag for production code.
365-365: Use typed event.
WithinonChange={(value) => { … }}, clarifyingvalue’s type can help avoid confusion. If “value” is a string, you can strongly type it to ensure safe usage.
463-463: Internationalize tooltip text.
"select_location_from"is already translated, but the entire tooltip text might need translation beyond that phrase. Confirm that all relevant text strings are also in I18n files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
public/locale/en.json(1 hunks)src/Utils/utils.ts(1 hunks)src/components/Facility/FacilityCreate.tsx(7 hunks)src/types/organization/organizationApi.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- public/locale/en.json
- src/Utils/utils.ts
🔇 Additional comments (4)
src/types/organization/organizationApi.ts (1)
65-81: Consider null or empty results handling.
This function returnsdata.results?.[0], which may beundefinedif no organizations match the parameters. Ensure the calling code handles a possibleundefinedresult.src/components/Facility/FacilityCreate.tsx (3)
75-76: Keep track of partial selection states.
TheselectedLevelsandpincodestates are logically related. Ifpincodeis changed or cleared, verify thatselectedLevelsremains consistent and does not reference outdated organizations.
239-244: ValidatestateOrganddistrictOrg.
If either call returns no data,stateOrgordistrictOrgwill be undefined. This might unintentionally resetselectedLevels. Confirm that you genuinely intend to discard partial matches if one level is missing.
377-381: Pass updated value for new facility creation.
value={facilityData?.geo_organization}will remainundefinedwhen creating a new facility. Confirm that this use case is acceptable or initialize it to an empty string to avoid react warnings.
|
@rithviknishad can you please review the changes |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes