-
Notifications
You must be signed in to change notification settings - Fork 995
Fix missing AbortController cleanup in useEffect hooks causing memory leaks #15061
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
- Add proper AbortController cleanup in MedicationBillForm.tsx useEffect - Add proper AbortController cleanup in DispenseDrawer.tsx useEffect - Use TanStack Query signal parameter in NoteManager.tsx useInfiniteQuery - Remove improper signal in AccountSheet.tsx useMutation - Remove improper signal in TagConfigView.tsx useMutation - Add AbortController ref cleanup in SpecimenIDScanDialog.tsx event handler - Simplify DispensedMedicationList.tsx completeMedications mutation Co-authored-by: Jacobjeevan <[email protected]>
…n mutations - AccountSheet: Add signal parameter to query call in updateMutation - TagConfigView: Add signal parameter to query call in archiveMutation - DispensedMedicationList: Add signal parameter to query call in completeMedications Note: These are user-initiated mutations that should complete, so signal won't be aborted Co-authored-by: Jacobjeevan <[email protected]>
| try { | ||
| const promises = missingInventories.map( | ||
| async ([productKnowledgeId]) => { | ||
| const inventoriesResponse = await query(inventoryApi.list, { | ||
| pathParams: { facilityId, locationId: currentLocation.id }, | ||
| queryParams: { | ||
| limit: 100, | ||
| product_knowledge: productKnowledgeId, | ||
| net_content_gt: 0, | ||
| }, | ||
| })({ signal: abortController.signal }); | ||
|
|
||
| return { | ||
| productKnowledgeId, | ||
| inventories: inventoriesResponse.results || [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we doing query manually in a useEffect when we have tanstack query?
we can simply use useQueries instead
| signal, | ||
| body: { datapoints: updates }, | ||
| })({ signal }); | ||
| })({ signal: new AbortController().signal }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the point when new abort controller signals are made each time mutate is called?
Proposed Changes
Multiple components created
new AbortController().signalwithout storing the controller reference, preventing cleanup. This caused memory leaks (requests continuing after unmount), race conditions (stale requests updating state), and React warnings.useEffect fixes (critical):
MedicationBillForm.tsx,DispenseDrawer.tsx: Store AbortController, add cleanup function, checksignal.abortedbefore setStateTanStack Query fixes:
NoteManager.tsx: Use destructuredsignalfrom queryFn context instead of creating new controllerAccountSheet.tsx,TagConfigView.tsx,DispensedMedicationList.tsx: Add required signal parameter to query calls in mutationsEvent handler fix:
SpecimenIDScanDialog.tsx: Use useRef to persist AbortController, cleanup on unmount, abort previous requestsBefore:
After:
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.