Fix: Duplicate network requests in Patient Details page #9297
Conversation
WalkthroughThe pull request includes several modifications across multiple components related to patient management. Key changes involve streamlining the handling of 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 (2)
src/components/Facility/PatientNotesList.tsx (1)
Line range hint
31-54: Consider implementing request debouncingThe
fetchNotesfunction is called in multiple scenarios (thread changes, reload, pagination). To further prevent duplicate requests, consider implementing debouncing.Here's a suggested improvement:
import { debounce } from 'lodash'; const debouncedFetchNotes = debounce(async () => { setIsLoading(true); const { data }: any = await request(routes.getPatientNotes, { pathParams: { patientId: props.patientId }, query: { offset: (state.cPage - 1) * RESULTS_PER_PAGE_LIMIT, thread, }, }); setState((prevState: any) => ({ ...prevState, notes: state.cPage === 1 ? data.results : [...prevState.notes, ...data.results], totalPages: Math.ceil(data.count / pageSize), })); setIsLoading(false); setReload(false); }, 300);src/components/Patient/PatientHome.tsx (1)
68-72: Consider using React 18's useEffect cleanup.While the mount state management works, it could be simplified using React 18's useEffect cleanup pattern:
-const [isPatientHomeMounted, setIsPatientHomeMounted] = useState(false); - -useEffect(() => { - setIsPatientHomeMounted(true); -}, []); +const [isMounted, setIsMounted] = useState(false); + +useEffect(() => { + setIsMounted(true); + return () => setIsMounted(false); +}, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/components/Facility/PatientNotesList.tsx(1 hunks)src/components/Patient/PatientDetailsTab/EncounterHistory.tsx(1 hunks)src/components/Patient/PatientDetailsTab/Notes.tsx(3 hunks)src/components/Patient/PatientDetailsTab/index.tsx(1 hunks)src/components/Patient/PatientHome.tsx(3 hunks)
🔇 Additional comments (5)
src/components/Patient/PatientDetailsTab/index.tsx (1)
14-14: Verify consistent implementation of refetch across components
The addition of refetch method to PatientProps is a good approach for centralized data refreshing. However, we should ensure it's properly implemented across all components to prevent duplicate requests.
✅ Verification successful
Let me try a different approach to verify the implementation of refetch across components.
Let me search specifically for the implementation of refetch in the patient components.
Based on the evidence gathered, I can now provide a final response:
Implementation of refetch is consistent across patient components
The refetch method in PatientProps interface is properly implemented and used consistently across all patient-related components. Key findings:
- The interface is correctly defined in
src/components/Patient/PatientDetailsTab/index.tsxwithrefetch: () => void - All components using PatientProps (Demography, EncounterHistory, HealthProfileSummary, etc.) receive and handle the refetch prop appropriately
- EncounterHistory component explicitly uses refetch for data refresh functionality
- The pattern is consistent with React's data fetching best practices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using refetch to verify consistent implementation
ast-grep --pattern 'const $_ = ($props: PatientProps) => {
$$$
const { refetch } = props;
$$$
}'
Length of output: 104
Script:
#!/bin/bash
# First, let's find all files that import or use PatientProps
rg "PatientProps" -l
# Then, let's look for any usage of refetch in these components
rg "refetch" -A 3 -B 3
Length of output: 70051
Script:
#!/bin/bash
# Search for PatientProps interface definition and its usage
ast-grep --pattern 'interface PatientProps {
$$$
}'
# Search for components using PatientProps with refetch
rg -A 5 -B 5 "const \w+ = \(.*: PatientProps\)"
Length of output: 7866
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx (1)
16-16: LGTM! Improved data fetching efficiency
The removal of separate loading state and useQuery hook, combined with passing refetch to ConsultationCard, helps prevent duplicate requests while maintaining proper data refresh capabilities.
src/components/Patient/PatientDetailsTab/Notes.tsx (2)
28-29: LGTM! Props interface simplification.
The change to use PatientProps instead of a custom interface reduces code duplication and ensures consistency across components.
139-139: LGTM! Consistent disabled state management.
Using patientData.is_active directly for both the input field and button ensures consistent behavior and reduces unnecessary state management.
Also applies to: 147-147
src/components/Patient/PatientHome.tsx (1)
477-477: LGTM! Proper data refresh handling.
Adding the refetch prop to child components aligns with the PR objective of fixing duplicate network requests by enabling controlled data refresh.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/components/Patient/PatientDetailsTab/Demography.tsx (1)
72-76: Consider adding error handling for refetch operationWhile the implementation correctly triggers a refetch when the patient ID changes, it would be beneficial to handle potential refetch failures.
Consider updating the implementation to handle errors:
useEffect(() => { if (patientData.id) { - refetch(); + refetch().catch((error) => { + Notification.Error({ + msg: t("failed_to_refresh_insurance_details"), + }); + }); } }, [patientData.id, refetch]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/Facility/PatientNotesList.tsx(1 hunks)src/components/Patient/PatientDetailsTab/Demography.tsx(1 hunks)src/components/Patient/PatientHome.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Patient/PatientHome.tsx
- src/components/Facility/PatientNotesList.tsx
🔇 Additional comments (1)
src/components/Patient/PatientDetailsTab/Demography.tsx (1)
64-70: LGTM: Optimized insurance details fetching
The changes to prevent automatic prefetching align well with the PR's objective of reducing duplicate network requests. Setting prefetch: false ensures that the data is only fetched when explicitly needed.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Patient/PatientDetailsTab/Demography.tsx (1)
72-76: Consider debouncing the refetch call.The current implementation might trigger multiple API calls if
patientData.idchanges rapidly. Consider debouncing the refetch call to prevent potential race conditions.+import { useCallback } from 'react'; +import debounce from 'lodash/debounce'; +const debouncedRefetch = useCallback( + debounce((fn) => fn(), 300), + [] +); useEffect(() => { if (patientData.id) { - refetch(); + debouncedRefetch(refetch); } }, [patientData.id, refetch]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/components/Facility/PatientNotesList.tsx(1 hunks)src/components/Patient/PatientDetailsTab/Demography.tsx(1 hunks)src/components/Patient/PatientDetailsTab/EncounterHistory.tsx(1 hunks)src/components/Patient/PatientDetailsTab/Notes.tsx(3 hunks)src/components/Patient/PatientDetailsTab/index.tsx(1 hunks)src/components/Patient/PatientHome.tsx(1 hunks)
🔇 Additional comments (5)
src/components/Facility/PatientNotesList.tsx (1)
61-64: LGTM! Successfully implemented the suggested optimization.
The useEffect hooks have been combined as suggested, which should prevent duplicate requests by having a single effect respond to both reload and thread changes.
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx (1)
16-16: Verify the refetch prop usage in ConsultationCard.
The refetch prop is now passed to ConsultationCard. Please ensure that ConsultationCard component properly handles this prop to prevent any potential memory leaks or unnecessary re-renders.
✅ Verification successful
From the search results, I can see that the ConsultationCard component is imported from "@/components/Facility/ConsultationCard" and the refetch prop is properly passed to it. Let me verify its implementation with another search.
Based on the code inspection, I can now generate the final response:
ConsultationCard properly handles the refetch prop
The ConsultationCard component correctly defines and uses the refetch prop in its interface and implementation. The prop is properly typed as refetch: () => void and is used appropriately in the Beds component for refreshing patient data. There are no potential memory leaks or unnecessary re-renders as the prop is only passed down and used as a callback.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how ConsultationCard uses the refetch prop
ast-grep --pattern 'interface ConsultationCardProps {
$$$
refetch: $_
$$$
}'
Length of output: 84
Script:
#!/bin/bash
# Let's try a broader search to find ConsultationCard component and its props
ast-grep --pattern 'const ConsultationCard = ({ $$ }) => {
$$$
}'
# Also search for any usage of refetch within ConsultationCard
rg -A 5 "refetch" --type typescript
Length of output: 140
Script:
#!/bin/bash
# Search for ConsultationCard component definition and its usage
rg -A 10 "ConsultationCard"
# Search for refetch usage
rg -A 5 "refetch"
# Find the ConsultationCard file
fd ConsultationCard
Length of output: 69304
Script:
#!/bin/bash
# Search for ConsultationCard component implementation and its props interface
cat src/components/Facility/ConsultationCard.tsx
Length of output: 6569
src/components/Patient/PatientDetailsTab/Notes.tsx (1)
28-29: LGTM! Improved efficiency by removing redundant state management.
The changes effectively reduce duplicate network requests by:
- Using
PatientPropsto access patient data from parent - Removing redundant state management for patient's active status
- Directly using
patientData.is_activeinstead of maintaining separate state
Also applies to: 139-139, 147-147
src/components/Patient/PatientDetailsTab/Demography.tsx (1)
64-70: LGTM! Improved data fetching strategy.
The changes effectively reduce duplicate network requests by:
- Preventing automatic prefetch with
prefetch: false - Controlling when insurance details are fetched
src/components/Patient/PatientHome.tsx (1)
471-471: LGTM! Added refetch prop for coordinated data refresh.
The addition of the refetch prop enables child components to trigger data refresh in a coordinated manner, supporting the overall goal of reducing duplicate requests.
|
@rithviknishad Any further changes required? |
|
Hey @rithviknishad can you please update the review |
| const { data: insuranceDetials, refetch } = useQuery( | ||
| routes.hcx.policies.list, | ||
| { | ||
| query: { patient: id }, | ||
| prefetch: false, // Don't prefetch by default | ||
| }, | ||
| }); | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (patientData.id) { | ||
| refetch(); | ||
| } | ||
| }, [patientData.id, refetch]); |
There was a problem hiding this comment.
why disable prefetch? wouldn't patientData.id always be present?
There was a problem hiding this comment.
In PatientHome.tsx, the getPatient API fetches patientData and passes it to Demography as prop.
Earlier, I disabled prefetch for the insuranceDetails API in Demography to avoid calls with an undefined patientData.id.
Now, by setting prefetch to !!patientData.id in the query itself, the API triggers only when the patientData.id is available,This prevents unnecessary call.
This change working correctly. Please confirm if this correct.
There was a problem hiding this comment.
you can get the patient id using the useSlug or by passing it as a prop right? We can prevent request waterfalls this way. There's no reason for fetching insurance details to wait until we get the patient object right?
There was a problem hiding this comment.
Ok i will check it .
There was a problem hiding this comment.
you can get the patient id using the
useSlugor by passing it as a prop right? We can prevent request waterfalls this way. There's no reason for fetching insurance details to wait until we get the patient object right?
Yes we are passing patient id as prop from PatientHome.tsx. This patient id is fetched from getPatient api request.
But the tabs are rendered before the fetching is completed and props are passed as undefined to demography tab.
So to prevent this i have made fetching insurance details to wait until we get the property fetched patient object.
Instead of this should i write a conditional statement in patienthome.tsx so that tabs are not rendered until patientdata is fetched completely.
{Tab && patientData?.id && (
<Tab
facilityId={facilityId || ""}
id={patientData.id || ""}
patientData={patientData}
refetch={refetch}
/>
)}There was a problem hiding this comment.
We can get the patient ID from the URL right?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Patient/PatientHome.tsx (2)
Line range hint
65-72: Consider simplifying volunteer assignment state managementThe current implementation initializes state with a prop and updates it via an effect, which could lead to unnecessary re-renders. Consider using the prop directly or implementing a controlled component pattern.
-const [assignedVolunteer, setAssignedVolunteer] = useState< - AssignedToObjectModel | undefined ->(patientData.assigned_to_object); - -useEffect(() => { - setAssignedVolunteer(patientData.assigned_to_object); -}, [patientData.assigned_to_object]); +const [assignedVolunteer, setAssignedVolunteer] = useState< + AssignedToObjectModel | undefined +>(); + +// Use patientData.assigned_to_object directly in the UI +// Only update assignedVolunteer when explicitly changing it
Line range hint
82-102: Enhance error handling for network operationsThe volunteer assignment operation lacks proper error handling and user feedback for failure cases.
const handleAssignedVolunteer = async () => { const { res, data } = await request(routes.patchPatient, { pathParams: { id: patientData.id as string, }, body: { assigned_to: (assignedVolunteer as UserBareMinimum)?.id || null, }, }); - if (res?.ok && data) { + if (!res?.ok) { + Notification.Error({ + msg: t("volunteer_assignment_failed"), + }); + return; + } + if (data) { setPatientData(data); if (!assignedVolunteer) { Notification.Success({ msg: t("volunteer_assigned"), }); } else { Notification.Success({ msg: t("volunteer_unassigned"), }); } refetch(); } setOpenAssignVolunteerDialog(false); if (errors["assignedVolunteer"]) delete errors["assignedVolunteer"]; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Patient/PatientDetailsTab/Demography.tsx(2 hunks)src/components/Patient/PatientHome.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Patient/PatientDetailsTab/Demography.tsx
🔇 Additional comments (2)
src/components/Patient/PatientHome.tsx (2)
475-475: LGTM: Proper implementation of data fetching control
The addition of the refetch prop to Tab components allows controlled data refetching, which helps prevent duplicate network requests as intended in the PR objectives.
Line range hint 391-417: LGTM: Good accessibility implementation
The tab navigation implementation follows accessibility best practices with proper ARIA attributes and semantic HTML structure.
|
👋 Hi, @Mahendar0701, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
Hi @rithviknishad with the latest changes in useQuery.ts , the issues related to demograpgy tab seems to be got fixed. Here are the screenshots of tabs network calls
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx (1)
Line range hint
18-52: Consider memoizing the card render functionThe implementation effectively prevents duplicate network requests through proper use of PaginatedList. However, we could further optimize performance by memoizing the card render function.
Consider this optimization:
+ const renderConsultationCard = React.useCallback( + (item: ConsultationModel) => ( + <ConsultationCard + itemData={item} + isLastConsultation={ + !!patientData.last_consultation && + item.id === patientData.last_consultation.id + } + refetch={refetch} + /> + ), + [patientData.last_consultation, refetch] + ); return ( <PaginatedList route={routes.getConsultationList} query={{ patient: id }} perPage={5} > {(_) => ( <div className="mt-8"> <PaginatedList.WhenLoading> <CircularProgress /> </PaginatedList.WhenLoading> <PaginatedList.WhenEmpty className="py-2"> {/* ... */} </PaginatedList.WhenEmpty> <PaginatedList.Items<ConsultationModel>> - {(item) => ( - <ConsultationCard - itemData={item} - isLastConsultation={ - !!patientData.last_consultation && - item.id === patientData.last_consultation.id - } - refetch={refetch} - /> - )} + {renderConsultationCard} </PaginatedList.Items> {/* ... */} </div> )} </PaginatedList> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx(1 hunks)
🔇 Additional comments (2)
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx (2)
1-11: LGTM! Clean import organization
The imports are well-organized and contain only the necessary dependencies, which aligns with the PR's goal of reducing redundancy.
13-14: LGTM! Props handling improved
The simplified props destructuring improves code clarity and follows the previous review suggestions. The inclusion of the refetch prop enables proper data refresh handling.
I will get it done in PR #9377 as you mentioned earlier in that PR |
Proposed Changes
Fixes Duplicate network requests on patient details page #9269
Removed Duplicate calls of policy
Removed Duplicate calls of consultation details
Removed Duplicate calls of notes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes