-
Notifications
You must be signed in to change notification settings - Fork 525
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
Fix: FacilityData not populating on Edit #10077
Fix: FacilityData not populating on Edit #10077
Conversation
WalkthroughThe pull request introduces changes to query management in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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. |
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Facility/FacilityHome.tsx (2)
99-99
: Remove commented-out code.The commented-out
facilityFetch
is no longer needed since we're using query invalidation.- // refetch: facilityFetch,
138-140
: Consider adding error handling for query invalidation.While the cache invalidation strategy is solid, we should handle potential failures to ensure robust data synchronization.
queryClient.invalidateQueries({ queryKey: ["facility", facilityId], + onError: (error) => { + toast.error(t("error_refreshing_facility_data")); + console.error("Failed to invalidate facility query:", error); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/FacilityHome.tsx
(4 hunks)src/pages/Organization/components/EditFacilitySheet.tsx
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
src/pages/Organization/components/EditFacilitySheet.tsx (1)
40-40
: LGTM! Query key structure simplified for proper cache invalidation.The simplified query key structure ensures consistent cache invalidation across components and aligns with React Query's best practices.
src/components/Facility/FacilityHome.tsx (1)
138-140
: LGTM! Consistent cache invalidation strategy implemented.The implementation properly invalidates the facility query after image operations, ensuring the UI reflects the latest data.
Also applies to: 160-162
Hey Team, Don't you think we should show |
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Facility/FacilityForm.tsx (3)
Line range hint
171-175
: Consider sanitizing the geolocation error message.The current implementation directly concatenates the error message from the Geolocation API, which could potentially lead to XSS vulnerabilities. Consider using a safe error message handling approach.
- toast.error(t("unable_to_get_location") + error.message); + toast.error(t("unable_to_get_location_with_reason", { reason: error.code === 1 ? "PERMISSION_DENIED" : error.code === 2 ? "POSITION_UNAVAILABLE" : "TIMEOUT" }));
122-130
: Consider refining the form reset behavior for updates.Currently, the form is reset after both create and update operations. For updates, this might not be the desired behavior as users might want to make additional edits.
onSuccess: (_data: FacilityModel) => { toast.success(t("facility_updated_successfully")); queryClient.invalidateQueries({ queryKey: ["organizationFacilities"], }); queryClient.invalidateQueries({ queryKey: ["facility"], }); - form.reset(); onSubmitSuccess?.(); },
Line range hint
367-382
: Enhance accessibility for the geolocation button's loading state.While the button properly shows a loading state visually, its accessibility for screen readers could be improved by adding appropriate ARIA attributes.
<Button type="button" variant="outline" size="sm" onClick={handleGetCurrentLocation} disabled={isGettingLocation} + aria-busy={isGettingLocation} className="flex items-center gap-2" data-cy="get-location-button" > {isGettingLocation ? ( <> - <CareIcon icon="l-spinner" className="h-4 w-4 animate-spin" /> + <CareIcon icon="l-spinner" className="h-4 w-4 animate-spin" aria-hidden="true" /> {t("getting_location")} </> ) : ( <> - <CareIcon icon="l-location-point" className="h-4 w-4" /> + <CareIcon icon="l-location-point" className="h-4 w-4" aria-hidden="true" /> {t("get_current_location")} </> )} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/FacilityForm.tsx
(1 hunks)src/pages/Organization/components/EditFacilitySheet.tsx
(0 hunks)
💤 Files with no reviewable changes (1)
- src/pages/Organization/components/EditFacilitySheet.tsx
🔇 Additional comments (1)
src/components/Facility/FacilityForm.tsx (1)
122-127
: LGTM! The additional query invalidation ensures data consistency.The addition of
["facility"]
query invalidation complements the existing["organizationFacilities"]
invalidation, ensuring that both the facility list and individual facility data are refreshed after updates. This change effectively addresses the issue of FacilityData not populating correctly during edit operations.
"cover_image_deleted.": "Cover image deleted.", | ||
"cover_image_updated.": "Cover image updated.", | ||
"cover_image_deleted": "Cover Image Deleted", | ||
"cover_image_updated": "Cover Image Updated", |
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.
nice find, how did this even end up here 🤣
@AdityaJ2305 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
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
useQueryClient
hook.Bug Fixes
Chores