Show Readable Names in Breadcrumbs Instead of Using External Identifiers #10015
Conversation
WalkthroughThe pull request introduces a consistent approach to fetching facility data across multiple components using React Query's Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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: 0
🧹 Nitpick comments (10)
src/components/Users/UserHome.tsx (1)
72-72: Consider optimizing the loading check.The loading check includes
facilityQuery.isLoadingunconditionally, but it might be unnecessary whenfacilityIdis not provided.- if (loading || facilityQuery.isLoading || !userData) { + if (loading || (props.facilityId && facilityQuery.isLoading) || !userData) {src/components/Facility/FacilityUsers.tsx (1)
127-134: Consider adding error handling for the facility query.While the query is well-structured, it would benefit from error handling to gracefully handle failed facility data fetches.
const facilityQuery = useQuery({ queryKey: ["facility", props.facilityId], queryFn: query(routes.getPermittedFacility, { pathParams: { id: props.facilityId, }, }), + onError: (error) => { + console.error('Failed to fetch facility:', error); + }, });src/components/Patient/PatientHome.tsx (1)
29-36: Consider adding error boundaries for facility data fetches.While the query implementation is correct, adding error boundaries would improve the user experience when facility data fetches fail.
const facilityQuery = useQuery({ queryKey: ["facility", props.facilityId], queryFn: query(routes.getPermittedFacility, { pathParams: { id: props.facilityId || "", }, }), + onError: (error) => { + console.error('Failed to fetch facility:', error); + }, });src/pages/Encounters/EncounterShow.tsx (1)
95-102: Add error handling and loading states for the facility query.The facility query implementation looks good, but it should handle error and loading states to ensure a smooth user experience.
const facilityQuery = useQuery({ queryKey: ["facility", props.facilityId], queryFn: query(routes.getPermittedFacility, { pathParams: { id: props.facilityId, }, }), + retry: false, + staleTime: 5 * 60 * 1000, // Cache for 5 minutes since facility data rarely changes });src/pages/Appointments/BookAppointment.tsx (1)
71-86: Add error handling and enable queries conditionally.The queries look good but could be improved with error handling and conditional enabling.
const facilityQuery = useQuery({ queryKey: ["facility", props.facilityId], queryFn: query(routes.getPermittedFacility, { pathParams: { id: props.facilityId, }, }), + enabled: !!props.facilityId, + retry: false, + staleTime: 5 * 60 * 1000, }); const patientQuery = useQuery({ queryKey: ["patient", props.patientId], queryFn: query(routes.patient.getPatient, { pathParams: { id: props.patientId, }, }), + enabled: !!props.patientId, + retry: false, });src/components/Patient/PatientRegistration.tsx (1)
160-167: Add error handling and enable query conditionally.The facility query implementation could be improved with error handling and conditional enabling.
const facilityQuery = useQuery({ queryKey: ["facility", props.facilityId], queryFn: query(routes.getPermittedFacility, { pathParams: { id: props.facilityId, }, }), + enabled: !!props.facilityId, + retry: false, + staleTime: 5 * 60 * 1000, });src/pages/Encounters/EncounterList.tsx (2)
205-212: Improve error handling and remove empty string fallback.The facility query implementation could be improved:
- Add error handling
- Remove empty string fallback for facilityId
const facilityQuery = useQuery({ queryKey: ["facility", facilityId], queryFn: query(routes.getPermittedFacility, { pathParams: { - id: facilityId || "", + id: facilityId, }, }), + enabled: !!facilityId, + retry: false, + staleTime: 5 * 60 * 1000, });
248-254: Remove empty string fallback in breadcrumb replacements.The implementation should avoid using empty string fallback for facilityId.
<Page title={t("encounters")} hideBack={true} crumbsReplacements={{ - [facilityId || ""]: { name: facilityQuery.data?.name }, + [facilityId]: { name: facilityQuery.data?.name }, }} >src/pages/FacilityOrganization/FacilityOrganizationIndex.tsx (2)
39-45: LGTM! Consider adding error handling.The facility data query is well-structured and aligns with React Query best practices. However, consider handling potential error states to provide a better user experience when the facility data fetch fails.
const { data: facilityData, isLoading: isFacilityLoading } = useQuery({ queryKey: ["facility", facilityId], queryFn: query(routes.getPermittedFacility, { pathParams: { id: facilityId }, }), enabled: !!facilityId, + onError: (error) => { + // Handle error state, e.g., show a toast notification + console.error('Failed to fetch facility data:', error); + } });
97-103: Add optional chaining for safer property access.While the breadcrumb replacement logic is clean, we should handle the case where
facilityDatamight be undefined.<Page title={t("facility_organizations")} hideBack={true} crumbsReplacements={{ - [facilityId]: { name: facilityData?.name }, + [facilityId]: facilityData && { name: facilityData.name }, }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/Facility/FacilityUsers.tsx(2 hunks)src/components/Patient/EncounterQuestionnaire.tsx(3 hunks)src/components/Patient/PatientHome.tsx(2 hunks)src/components/Patient/PatientRegistration.tsx(2 hunks)src/components/Users/UserHome.tsx(4 hunks)src/pages/Appointments/BookAppointment.tsx(3 hunks)src/pages/Encounters/EncounterList.tsx(2 hunks)src/pages/Encounters/EncounterShow.tsx(2 hunks)src/pages/FacilityOrganization/FacilityOrganizationIndex.tsx(2 hunks)
🔇 Additional comments (9)
src/components/Patient/EncounterQuestionnaire.tsx (2)
32-41: LGTM! Well-structured query implementation.The encounter query is well-implemented with proper error handling and conditional enabling.
47-57: LGTM! Comprehensive breadcrumb replacements.The implementation efficiently uses the encounter data to replace multiple identifiers with readable names in a single query.
src/components/Users/UserHome.tsx (1)
64-70: LGTM! Well-structured facility query.The facility query is properly implemented with correct path parameters.
src/components/Patient/PatientHome.tsx (1)
60-63: LGTM! Well-structured breadcrumb replacements.The implementation correctly combines both patient and facility names in the breadcrumbs.
src/pages/Encounters/EncounterShow.tsx (1)
205-207: LGTM! Clean implementation of breadcrumb replacements.The implementation safely handles undefined facility data using optional chaining.
src/pages/Appointments/BookAppointment.tsx (1)
224-230: LGTM! Clean implementation of breadcrumb replacements.The implementation safely handles undefined data using optional chaining for both facility and patient names.
src/components/Patient/PatientRegistration.tsx (1)
295-300: LGTM! Clean implementation of breadcrumb replacements.The implementation safely handles undefined facility data using optional chaining.
src/pages/FacilityOrganization/FacilityOrganizationIndex.tsx (2)
46-46: LGTM! Comprehensive loading state handling.The combined loading check ensures a smooth user experience by showing the skeleton UI until all data is available.
39-45: Verify consistent implementation across components.Let's verify that this breadcrumb enhancement pattern is consistently implemented across other components mentioned in the AI summary.
Also applies to: 97-103
✅ Verification successful
Implementation verified as consistent across components
The facility data query implementation follows a consistent pattern across the codebase, with proper query keys and conditional fetching. The variations found in some components appear to be intentional based on different access requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent implementation of facility name in breadcrumbs # across components mentioned in the AI summary # Search for similar useQuery patterns with facility data echo "Checking for consistent facility data query implementation..." rg -l "useQuery.*getPermittedFacility.*crumbsReplacements" \ src/components/Facility/FacilityUsers.tsx \ src/components/Patient/PatientHome.tsx \ src/components/Patient/PatientRegistration.tsx \ src/pages/Appointments/BookAppointment.tsx # Search for potential performance bottlenecks echo "Checking for potential duplicate queries..." rg "queryKey.*facility.*facilityId" -A 2Length of output: 6170
|
@rithviknishad Can you please check this . I had to close the other PR related to this. |
|
👋 Hi, @DonXavierdev, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
…o issues/9822/Replace-IDs-Breadcrumbs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pages/Encounters/EncounterList.tsx (1)
254-260: Consider handling loading and error states for breadcrumb replacements.While the implementation works, it might be good to handle loading and error states for a better user experience.
Consider adding a loading state and fallback:
<Page title={t("encounters")} hideBack={true} crumbsReplacements={{ - [facilityId || ""]: { name: facilityQuery.data?.name }, + [facilityId || ""]: { + name: facilityQuery.data?.name, + loading: facilityQuery.isLoading, + error: facilityQuery.error?.message + }, }} >src/components/Patient/PatientRegistration.tsx (1)
320-325: Consider handling loading and error states for breadcrumb replacements.While the implementation works, it might be good to handle loading and error states for a better user experience.
Consider adding a loading state and fallback:
<Page title={title} crumbsReplacements={{ - [facilityId]: { name: facilityQuery.data?.name }, + [facilityId]: { + name: facilityQuery.data?.name, + loading: facilityQuery.isLoading, + error: facilityQuery.error?.message + }, }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Patient/PatientRegistration.tsx(2 hunks)src/pages/Encounters/EncounterList.tsx(2 hunks)
🔇 Additional comments (2)
src/pages/Encounters/EncounterList.tsx (1)
214-221: LGTM! The facility query implementation is consistent.The implementation follows the established pattern for facility data fetching across components.
src/components/Patient/PatientRegistration.tsx (1)
168-176: LGTM! The facility query implementation is consistent.The implementation follows the established pattern for facility data fetching across components.
Another way would be to pass it via props but then still i would have to fetch it at some point right ? @bodhish |
nihal467
added
discussion required
and removed
needs testing
needs review
needs-triage
labels
|
👋 Hi, @DonXavierdev, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
Let's hold this for now, if we switch to tanstack router, this can be achieved easily with route contexts |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Improvements