Enhancement of Shift patient Tab #9434
Conversation
WalkthroughThis pull request introduces comprehensive changes to the patient shifting and consultation management system. The modifications span across multiple components, focusing on enhancing the user interface, routing, and functionality of patient shifting processes. Key changes include restructuring the shifting routes, updating the patient details tab, introducing a new referral letter component, and refining the shift creation and details update interfaces. Changes
Assessment against linked issues
Suggested Labels
Suggested Reviewers
Poem
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. |
Mahendar0701
changed the title
Mahendar0701
changed the title
|
👋 Hi, @Mahendar0701, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
…hendar0701/care_fe into shift-details-tab-enhancement
![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 (2)
src/components/Shifting/ReferralLetter.tsx (2)
38-196: Enhance accessibility for screen readersThe component could benefit from improved accessibility:
- Add appropriate ARIA labels to the QR code
- Add semantic heading levels
- Improve the structure of data presentation for screen readers
<QRCodeSVG value={`${window.location.origin}/shifting/${data.id}`} size={120} + aria-label={t("scan_qr_code_for_patient_details")} className="mt-4 sm:mt-0" /> -<h3 className="mb-2 font-semibold">{t("patient_details")}</h3> +<h2 className="mb-2 font-semibold" role="heading" aria-level="2">{t("patient_details")}</h2> -<div className="grid grid-cols-1 gap-6 sm:grid-cols-2 text-lg"> +<div className="grid grid-cols-1 gap-6 sm:grid-cols-2 text-lg" role="region" aria-label={t("patient_information")}>
21-198: Optimize component performance with memoizationConsider memoizing the component to prevent unnecessary re-renders, especially since it handles complex data transformations and rendering.
-export const ReferralLetter = ({ id }: ReferralLetterProps) => { +export const ReferralLetter = React.memo(({ id }: ReferralLetterProps) => { // ... component implementation ... -}; +});Also consider memoizing expensive computations:
+const getPatientGender = React.useMemo( + () => GENDER_TYPES.find((i) => i.id === patientData?.gender)?.text, + [patientData?.gender] +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Patient/PatientDetailsTab/ShiftingHistory.tsx(3 hunks)src/components/Shifting/ReferralLetter.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Patient/PatientDetailsTab/ShiftingHistory.tsx
🔇 Additional comments (3)
src/components/Shifting/ReferralLetter.tsx (3)
21-36: Add error handling for failed API calls
The component should handle API errors to provide feedback to users when data fetching fails.
58-62: Improve URL handling for QR code
The direct use of window.location.origin could cause issues in SSR environments.
129-131: Add null check for consultation date access
The current implementation might throw runtime errors if consultation is undefined.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Shifting/ReferralLetter.tsx (1)
108-117: Consider adding a loading state for address details.The component should handle loading states for nested address objects to prevent potential undefined property access.
{patientData?.nationality === "India" && ( <> <p> - {patientData?.ward_object?.name},{" "} - {patientData?.local_body_object?.name} + {patientData?.ward_object?.name ?? t("not_available")},{" "} + {patientData?.local_body_object?.name ?? t("not_available")} </p> - <p>{patientData?.district_object?.name || "-"}</p> - <p>{patientData?.state_object?.name}</p> + <p>{patientData?.district_object?.name ?? t("not_available")}</p> + <p>{patientData?.state_object?.name ?? t("not_available")}</p> </> )}src/components/Shifting/ShiftingTable.tsx (3)
272-280: Improve authorization check clarity.The condition for enabling the transfer action combines multiple checks in a way that's hard to read and maintain.
+const canTransferPatient = (shift: ShiftingModel, user: any) => { + const isAdminUser = ["DistrictAdmin", "StateAdmin"].includes(user.user_type); + const isAssignedFacility = user.home_facility_object?.id === shift.assigned_facility; + const isPatientTransferAllowed = shift.patient_object.allow_transfer; + + return isPatientTransferAllowed && (isAdminUser || isAssignedFacility); +}; disabled={ - !shift.patient_object.allow_transfer || - !( - ["DistrictAdmin", "StateAdmin"].includes( - authUser.user_type, - ) || - authUser.home_facility_object?.id === - shift.assigned_facility - ) + !canTransferPatient(shift, authUser) }
102-117: Consider extracting ShiftRow into a separate component.The row rendering logic is complex and would benefit from being extracted into a separate component for better maintainability.
+interface ShiftRowProps { + shift: ShiftingModel; + hidePatient?: boolean; + currentPath: string; +} + +const ShiftRow = ({ shift, hidePatient, currentPath }: ShiftRowProps) => { + return ( + <div key={`shift_${shift.id}`} className="w-full"> + {/* ... existing row content ... */} + </div> + ); +}; {data?.map((shift: ShiftingModel) => ( - <div key={`shift_${shift.id}`} className="w-full"> - {/* ... existing row content ... */} - </div> + <ShiftRow + key={`shift_${shift.id}`} + shift={shift} + hidePatient={hidePatient} + currentPath={currentPath} + /> ))}
37-38: Consider using a custom hook for slide-over state management.The slide-over state management could be extracted into a custom hook for reusability across components.
+const useSlideOver = (initialState = false) => { + const [isOpen, setIsOpen] = useState(initialState); + const open = () => setIsOpen(true); + const close = () => setIsOpen(false); + const toggle = () => setIsOpen(!isOpen); + return { isOpen, open, close, toggle }; +}; -const [isSlideOverOpen, setIsSlideOverOpen] = useState(false); +const { isOpen: isSlideOverOpen, open: openSlideOver, close: closeSlideOver } = useSlideOver();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CAREUI/misc/PrintPreview.tsx(1 hunks)src/components/Shifting/ReferralLetter.tsx(1 hunks)src/components/Shifting/ShiftingTable.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CAREUI/misc/PrintPreview.tsx
🔇 Additional comments (4)
src/components/Shifting/ReferralLetter.tsx (3)
129-131:
Add null check for consultation date access.
The current implementation might throw runtime errors if consultation is undefined.
- {formatDateTime(
- consultation.encounter_date || consultation.created_date,
- ) || "-"}
+ {consultation?.encounter_date || consultation?.created_date
+ ? formatDateTime(
+ consultation.encounter_date || consultation.created_date
+ )
+ : "-"}Likely invalid or redundant comment.
21-36: 🛠️ Refactor suggestion
Add error handling for failed API calls.
While loading states are handled properly, the component should also handle API errors to provide feedback to users when data fetching fails.
- const { data, loading } = useTanStackQueryInstead(routes.getShiftDetails, {
+ const { data, loading, error } = useTanStackQueryInstead(routes.getShiftDetails, {
pathParams: { id },
});
const patientData = data?.patient_object;
const consultation = data?.patient_object
?.last_consultation as ConsultationModel;
const patientGender = GENDER_TYPES.find(
(i) => i.id === patientData?.gender,
)?.text;
- if (loading || !patientData) {
+ if (loading) {
return <Loading />;
}
+
+ if (error) {
+ return (
+ <div className="text-center text-red-500">
+ {t("failed_to_load_referral_letter")}
+ </div>
+ );
+ }
+
+ if (!patientData) {
+ return (
+ <div className="text-center text-gray-500">
+ {t("no_patient_data_available")}
+ </div>
+ );
+ }Likely invalid or redundant comment.
58-62: 🛠️ Refactor suggestion
Improve URL handling for QR code generation.
The direct use of window.location.origin could cause issues in SSR environments and makes testing more difficult.
+const getShiftingUrl = (id: string) => {
+ if (typeof window === 'undefined') return '';
+ return new URL(`/shifting/${id}`, window.location.origin).toString();
+};
<QRCodeSVG
- value={`${window.location.origin}/shifting/${data.id}`}
+ value={getShiftingUrl(data.id)}
size={120}
className="mt-4 sm:mt-0"
/>Likely invalid or redundant comment.
src/components/Shifting/ShiftingTable.tsx (1)
204-214: 🛠️ Refactor suggestion
Enhance dropdown menu accessibility.
The dropdown menu implementation needs accessibility improvements:
- Missing aria-label for the trigger button
- "..." is not descriptive for screen readers
<DropdownMenuTrigger asChild>
<h1
- className="w-7 hover:cursor-pointer font-bold text-lg text-primary-700"
+ className="w-7 hover:cursor-pointer font-bold text-lg text-primary-700"
+ aria-label={t("shift_actions_menu")}
onClick={(e) => {
e.stopPropagation();
e.preventDefault();
}}
>
- . . .
+ <span aria-hidden="true">. . .</span>
+ <span className="sr-only">{t("shift_actions_menu")}</span>
</h1>
</DropdownMenuTrigger>Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Patient/ShiftCreate.tsx (1)
282-292: 🛠️ Refactor suggestionImprove accessibility for form fields
The form fields are missing important accessibility attributes.
<TextFormField {...field("refering_facility_contact_name")} label="Name of Contact person at the current facility" required + aria-required="true" + aria-describedby="contact-name-error" /> <PhoneNumberFormField {...field("refering_facility_contact_number")} label="Contact person phone number" required + aria-required="true" + aria-describedby="contact-number-error" types={["mobile", "landline"]} />
🧹 Nitpick comments (5)
src/components/Patient/ShiftCreate.tsx (3)
41-44: Add TypeScript types for props interfaceThe interface could be more type-safe.
interface patientShiftProps { facilityId: string; patientId: string; - open: boolean; - setOpen: (state: boolean) => void; + open: boolean; + setOpen: React.Dispatch<React.SetStateAction<boolean>>; }
374-383: Add validation for ambulance contact detailsThe ambulance contact details fields should have proper validation when filled.
<TextFormField {...field("ambulance_driver_name")} label="Name of ambulance driver" + pattern="^[a-zA-Z\s]*$" + title="Please enter a valid name" /> <PhoneNumberFormField {...field("ambulance_phone_number")} label="Ambulance Phone Number" types={["mobile", "landline"]} + validateOnBlur /> <TextFormField {...field("ambulance_number")} label="Ambulance No." + pattern="^[A-Z0-9\s]*$" + title="Please enter a valid ambulance number" />
401-406: Add loading state to submit buttonThe submit button should show a loading state during form submission.
<Button variant="outline" onClick={() => goBack()}> {t("cancel")} </Button> -<Button variant="primary" onClick={() => handleSubmit()}> +<Button variant="primary" onClick={() => handleSubmit()} disabled={isLoading}> - {t("submit")} + {isLoading ? t("submitting") : t("submit")} </Button>src/components/Shifting/ShiftingCommentsSection.tsx (2)
26-46: LGTM! Well-implemented mutation logic with proper error handling.Consider adding more specific error messages in the error notification to help users understand what went wrong.
- Notification.Error({ msg: t("comment_add_error") }); + Notification.Error({ + msg: t("comment_add_error_with_details", { + error: error?.message || t("unknown_error") + }) + });
74-98: LGTM! Clean and user-friendly comment input implementation.Consider adding an aria-label to the submit button for better accessibility.
<ButtonV2 border={false} className="absolute right-2" ghost size="small" + aria-label={t("submit_comment")} onClick={async () => { await onSubmitComment(); query.refetch(); }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/Common/AuthorizedButton.tsx(1 hunks)src/components/Patient/ShiftCreate.tsx(4 hunks)src/components/Shifting/ReferralLetter.tsx(1 hunks)src/components/Shifting/ShiftDetailsUpdate.tsx(9 hunks)src/components/Shifting/ShiftingCommentsSection.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Common/AuthorizedButton.tsx
🔇 Additional comments (8)
src/components/Patient/ShiftCreate.tsx (1)
Line range hint 136-272: Switch to react-hook-form for better form management
The current form state management using useReducer is verbose and could be simplified. As suggested in previous reviews, switching to react-hook-form would provide:
- Built-in validation
- Better type safety
- Reduced boilerplate
- Better performance
Would you like me to help with migrating this form to use react-hook-form and shadcn components?
src/components/Shifting/ShiftingCommentsSection.tsx (2)
1-15: LGTM! Good migration to modern React practices.
The migration to @tanstack/react-query and addition of modern UI components improves the codebase quality.
130-131: Address the comment background contrast issue.
The current light gray background (bg-gray-400) with white text might have accessibility issues. This was previously flagged in the reviews.
- <div className="w-5/6 rounded-md border-secondary-300 bg-gray-400 p-3 text-white">
+ <div className="w-5/6 rounded-md border-secondary-300 bg-gray-600 p-3 text-white">src/components/Shifting/ReferralLetter.tsx (2)
60-64: Improve URL handling for QR code generation.
The direct use of window.location.origin could cause issues in SSR environments.
+const getShiftingUrl = (id: string) => {
+ if (typeof window === 'undefined') return '';
+ return new URL(`/shifting/${id}`, window.location.origin).toString();
+};
+
<QRCodeSVG
- value={`${window.location.origin}/shifting/${data.id}`}
+ value={getShiftingUrl(data.id)}
size={120}
className="mt-4 sm:mt-0"
/>131-133: Add null check for consultation date access.
The current implementation might throw runtime errors if consultation is undefined.
- {formatDateTime(
- consultation.encounter_date || consultation.created_date,
- ) || "-"}
+ {consultation?.encounter_date || consultation?.created_date
+ ? formatDateTime(
+ consultation.encounter_date || consultation.created_date
+ )
+ : "-"}src/components/Shifting/ShiftDetailsUpdate.tsx (3)
329-329: Avoid returning empty string during loading state.
Returning an empty string during loading could cause layout shifts and poor user experience.
- return "";
+ return (
+ <div className="flex h-full items-center justify-center">
+ <CircularProgress />
+ </div>
+ );366-371: Simplify complex option disabled logic.
The current implementation has complex nested conditions that could be simplified for better maintainability.
+const isOptionDisabled = (option: any, userType: string, homeFacility: string, assignedFacility: string) => {
+ const isAssignedToHomeFacility = homeFacility === assignedFacility;
+ const userTypeIndex = USER_TYPES.findIndex(type => type === userType);
+ const districtAdminIndex = USER_TYPES.findIndex(type => type === "DistrictAdmin");
+
+ return isAssignedToHomeFacility &&
+ userTypeIndex < districtAdminIndex &&
+ !["DESTINATION APPROVED"].includes(option.text);
+};
- optionDisabled={(option) =>
- home_facility === state.form.assigned_facility_object?.id &&
- USER_TYPES.findIndex((type) => user_type === type) <
- USER_TYPES.findIndex((type) => type === "DistrictAdmin") &&
- !["DESTINATION APPROVED"].includes(option.text)
- }
+ optionDisabled={(option) => isOptionDisabled(
+ option,
+ user_type,
+ home_facility,
+ state.form.assigned_facility_object?.id
+ )}581-587: Check button placement against Figma design.
Ensure the cancel/submit button placement matches the Figma design specifications.
#!/bin/bash
# Search for similar button layouts in the codebase for consistency
rg -A 2 'flex.*justify-end.*gap-2.*md:flex-row'
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Shifting/ShiftingCommentsSection.tsx (1)
129-130: 🛠️ Refactor suggestionImprove text contrast for better readability
The current background color (
bg-gray-400) might not provide sufficient contrast with white text. Consider using a darker shade for better readability.- <div className="w-5/6 rounded-md border-secondary-300 bg-gray-400 p-3 text-white"> + <div className="w-5/6 rounded-md border-secondary-300 bg-gray-600 p-3 text-white">
🧹 Nitpick comments (1)
src/components/Shifting/ShiftingCommentsSection.tsx (1)
27-47: Consider enhancing error handling with specific error messagesThe mutation implementation looks good and follows best practices. However, consider enhancing the error handling to provide more specific error messages based on the error type.
onError: () => { - Notification.Error({ msg: t("comment_add_error") }); + onError: (error: any) => { + const errorMessage = error?.response?.data?.detail || t("comment_add_error"); + Notification.Error({ msg: errorMessage }); + }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Shifting/ShiftingCommentsSection.tsx(3 hunks)
🔇 Additional comments (2)
src/components/Shifting/ShiftingCommentsSection.tsx (2)
1-16: LGTM! Import changes align with requirements
The switch from ButtonV2 to Button component has been implemented as requested, and all necessary imports are properly organized.
56-97: LGTM! Clean implementation with proper state handling
The component implementation is well-structured with appropriate loading states, empty states, and proper data refetching after mutations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/components/Shifting/ReferralLetter.tsx (2)
106-121: Extract address rendering logic for better maintainabilityThe address rendering logic is complex and could benefit from being extracted into a separate component or function.
+const AddressDetails = ({ patientData }) => { + if (!patientData) return null; + + return ( + <> + <p className="whitespace-pre-line"> + {patientData.address || "-"} + </p> + {patientData.nationality === "India" && ( + <> + <p> + {patientData.ward_object?.name}, {patientData.local_body_object?.name} + </p> + <p>{patientData.district_object?.name || "-"}</p> + <p>{patientData.state_object?.name}</p> + </> + )} + </> + ); +}; <div className="text-lg"> <h3 className="mb-2 font-semibold">{t("address")}</h3> - <p className="whitespace-pre-line"> - {patientData?.address || "-"} - </p> - {patientData?.nationality === "India" && ( - <> - <p> - {patientData?.ward_object?.name}, {patientData?.local_body_object?.name} - </p> - <p>{patientData?.district_object?.name || "-"}</p> - <p>{patientData?.state_object?.name}</p> - </> - )} + <AddressDetails patientData={patientData} /> </div>
22-200: Overall well-structured component with room for optimizationThe component demonstrates good practices with TypeScript, i18n, and UI organization. Consider these architectural improvements:
- Implement React Error Boundaries for better error handling
- Consider caching strategies for the API data
- Add proper loading skeletons instead of a generic loading spinner
- Consider implementing print-specific styles for the referral letter
src/components/Patient/ShiftCreate.tsx (1)
272-291: Enhance form accessibility for required fields.Required form fields should have proper ARIA attributes for better screen reader support.
<TextFormField {...field("refering_facility_contact_name")} label="Name of Contact person at the current facility" required + aria-required="true" + aria-describedby="contact-name-error" /> <PhoneNumberFormField {...field("refering_facility_contact_number")} label="Contact person phone number" required + aria-required="true" + aria-describedby="contact-number-error" types={["mobile", "landline"]} />src/components/Shifting/ShiftDetailsUpdate.tsx (1)
363-368: Simplify complex option disabled logic.Extract the complex nested conditions into a separate function for better maintainability.
+const isOptionDisabled = (option: any, userType: string, homeFacility: string, assignedFacility: string) => { + const isAssignedToHomeFacility = homeFacility === assignedFacility; + const userTypeIndex = USER_TYPES.findIndex(type => type === userType); + const districtAdminIndex = USER_TYPES.findIndex(type => type === "DistrictAdmin"); + + return isAssignedToHomeFacility && + userTypeIndex < districtAdminIndex && + !["DESTINATION APPROVED"].includes(option.text); +}; -optionDisabled={(option) => - home_facility === state.form.assigned_facility_object?.id && - USER_TYPES.findIndex((type) => user_type === type) < - USER_TYPES.findIndex((type) => type === "DistrictAdmin") && - !["DESTINATION APPROVED"].includes(option.text) -} +optionDisabled={(option) => isOptionDisabled( + option, + user_type, + home_facility, + state.form.assigned_facility_object?.id +)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Patient/ShiftCreate.tsx(4 hunks)src/components/Shifting/ReferralLetter.tsx(1 hunks)src/components/Shifting/ShiftDetailsUpdate.tsx(8 hunks)
🔇 Additional comments (7)
src/components/Shifting/ReferralLetter.tsx (4)
1-21: LGTM! Clean imports and proper TypeScript usage.
The code demonstrates good practices with:
- Well-organized imports
- Proper TypeScript interface definition
- Strong type safety for props
60-64: Improve URL handling for QR code
The direct use of window.location.origin could cause issues in SSR environments.
131-133: Add null check for consultation date access
The current implementation might throw runtime errors if consultation is undefined.
22-38: 🛠️ Refactor suggestion
Add error handling for API failures
The component should handle API errors to provide feedback to users when data fetching fails.
- const { data, isLoading } = useQuery({
+ const { data, isLoading, error } = useQuery({
queryKey: [routes.getShiftDetails.path, id],
queryFn: query(routes.getShiftDetails, { pathParams: { id } }),
});
const patientData = data?.patient_object;
const consultation = data?.patient_object
?.last_consultation as ConsultationModel;
const patientGender = GENDER_TYPES.find(
(i) => i.id === patientData?.gender,
)?.text;
- if (isLoading || !patientData) {
+ if (isLoading) {
return <Loading />;
}
+
+ if (error) {
+ return (
+ <Card className="shadow-none border-none">
+ <CardContent className="text-center text-red-500">
+ {t("failed_to_load_referral_letter")}
+ </CardContent>
+ </Card>
+ );
+ }
+
+ if (!patientData) {
+ return (
+ <Card className="shadow-none border-none">
+ <CardContent className="text-center text-gray-500">
+ {t("no_patient_data_available")}
+ </CardContent>
+ </Card>
+ );
+ }Likely invalid or redundant comment.
src/components/Patient/ShiftCreate.tsx (2)
337-343: LGTM: Patient category implementation is well-structured.
The patient category field properly handles value updates and includes required validation.
114-134: 🛠️ Refactor suggestion
Add error handling for the patient data query.
The query implementation should handle potential errors to provide better user feedback.
-const { data: patientData } = useQuery({
+const { data: patientData, error: patientError } = useQuery({
queryKey: [routes.getPatient.path, patientId],
queryFn: query(routes.getPatient, {
pathParams: { id: patientId },
}),
enabled: !!patientId,
});
+if (patientError) {
+ Notification.Error({ msg: "Failed to fetch patient data" });
+}Likely invalid or redundant comment.
src/components/Shifting/ShiftDetailsUpdate.tsx (1)
326-326: 🛠️ Refactor suggestion
Improve loading state handling.
Returning an empty string during loading could cause layout shifts. Consider using a loading indicator.
-return "";
+return (
+ <div className="flex h-full items-center justify-center">
+ <CircularProgress />
+ </div>
+);Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/components/Patient/ShiftCreate.tsx (2)
114-119: 🛠️ Refactor suggestionAdd error handling for the patient data query
The query implementation should handle potential errors to improve user experience.
- const { data: patientData } = useQuery({ + const { data: patientData, error: patientError } = useQuery({ queryKey: ["getPatient", patientId], queryFn: query(routes.getPatient, { pathParams: { id: patientId }, }), enabled: !!patientId, }); + if (patientError) { + Notification.Error({ msg: "Failed to fetch patient data" }); + }
272-408: 🛠️ Refactor suggestionForm layout needs accessibility improvements
The form layout looks good, but there are some accessibility concerns:
- Missing aria-required attributes on required fields
- No error message announcements for screen readers
Example fix for a form field:
<TextFormField {...field("refering_facility_contact_name")} label="Name of Contact person at the current facility" required + aria-required="true" + aria-describedby="contact-name-error" />
🧹 Nitpick comments (3)
src/components/Patient/ShiftCreate.tsx (1)
122-134: Improve null safety in useEffectThe current implementation might cause issues with optional chaining and type safety.
useEffect(() => { if (patientData) { - const patient_category = - patientData.last_consultation?.last_daily_round?.patient_category ?? - patientData.last_consultation?.category; + const patient_category = patientData.last_consultation + ? patientData.last_consultation.last_daily_round?.patient_category ?? + patientData.last_consultation.category + : undefined; const matchedCategory = PATIENT_CATEGORIES.find( - (c) => c.text === patient_category, + (c) => c.text === patient_category )?.id; - setPatientCategory(matchedCategory); + if (matchedCategory) { + setPatientCategory(matchedCategory); + } } }, [patientData]);src/components/Shifting/ReferralLetter.tsx (2)
50-195: Simplify nested conditional rendering.The component has deeply nested conditionals that could be simplified for better maintainability. Consider extracting the content into smaller components.
+ const PatientDetails = ({ patientData, patientGender }) => ( + <div> + <h3 className="mb-2 font-semibold">{t("patient_details")}</h3> + <p> + <span className="font-semibold leading-relaxed">{t("name")}: </span> + {patientData?.name} + </p> + {/* ... other patient details ... */} + </div> + ); + const AddressDetails = ({ patientData }) => ( + <div className="text-lg"> + <h3 className="mb-2 font-semibold">{t("address")}</h3> + <p className="whitespace-pre-line">{patientData?.address || "-"}</p> + {/* ... other address details ... */} + </div> + ); + const ConsultationDetails = ({ consultation, data }) => ( + <div className="grid grid-cols-1 gap-6 sm:grid-cols-2 text-lg"> + {/* ... consultation details ... */} + </div> + ); return ( <PrintPreview title={t("Patient Referral Letter")}> <Card className="shadow-none border-none"> {/* ... header ... */} <CardContent className="space-y-6"> {data && ( <div> - {/* ... current nested structure ... */} + <QRCodeAndLogo data={data} /> + <HospitalName data={data} /> + <div className="my-6 border-b-2" /> + <div className="grid grid-cols-1 gap-6 sm:grid-cols-2 text-lg"> + <PatientDetails patientData={patientData} patientGender={patientGender} /> + <AddressDetails patientData={patientData} /> + </div> + <div className="my-6 border-b-2" /> + <ConsultationDetails consultation={consultation} data={data} /> + <Footer /> </div> )} </CardContent> </Card> </PrintPreview> );
41-46: Enhance accessibility with semantic HTML and ARIA attributes.The component's accessibility could be improved with better semantic HTML and ARIA attributes.
- <PrintPreview title={t("Patient Referral Letter")}> + <PrintPreview + title={t("Patient Referral Letter")} + aria-label={t("referral_letter_document")} + > <Card className="shadow-none border-none"> - <CardHeader className="flex flex-col items-center space-y-4 sm:flex-row sm:justify-between sm:space-y-0"> + <CardHeader + className="flex flex-col items-center space-y-4 sm:flex-row sm:justify-between sm:space-y-0" + role="banner" + > <CardTitle className="mx-auto my-auto text-lg font-bold sm:text-2xl"> {t("referral_letter")} </CardTitle>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Patient/ShiftCreate.tsx(4 hunks)src/components/Shifting/ReferralLetter.tsx(1 hunks)src/components/Shifting/ShiftDetailsUpdate.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Shifting/ShiftDetailsUpdate.tsx
🔇 Additional comments (4)
src/components/Patient/ShiftCreate.tsx (2)
Line range hint 2-43: LGTM! Good migration to @tanstack/react-query
The changes align with the previous review feedback about switching to useQuery. The interface updates properly support the new SlideOver functionality.
Line range hint 135-271: Consider migrating to React Hook Form
The current form state management implementation is complex and could benefit from using React Hook Form for:
- Built-in validation
- Better type safety
- Reduced boilerplate
- Improved performance
Reference the examples in ScheduleExceptionForm and ScheduleTemplateForm for implementation details.
src/components/Shifting/ReferralLetter.tsx (2)
1-21: LGTM! Well-organized imports and proper type definitions.
The imports are logically grouped and the ReferralLetterProps interface properly types the id prop.
22-38: 🛠️ Refactor suggestion
Add error handling for failed API calls.
While loading states are handled properly, the component should handle API errors to provide feedback to users.
- const { data, isLoading } = useQuery({
+ const { data, isLoading, error } = useQuery({
queryKey: ["getShiftDetails", id],
queryFn: query(routes.getShiftDetails, { pathParams: { id } }),
});
const patientData = data?.patient_object;
const consultation = data?.patient_object
?.last_consultation as ConsultationModel;
const patientGender = GENDER_TYPES.find(
(i) => i.id === patientData?.gender,
)?.text;
- if (isLoading || !patientData) {
+ if (isLoading) {
return <Loading />;
}
+
+ if (error) {
+ return (
+ <div className="text-center text-red-500">
+ {t("failed_to_load_referral_letter")}
+ </div>
+ );
+ }
+
+ if (!patientData) {
+ return (
+ <div className="text-center text-gray-500">
+ {t("no_patient_data_available")}
+ </div>
+ );
+ }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
src/components/ui/textarea.tsx (2)
5-7: Add a short JSDoc for improved clarityIt's generally helpful to add a concise comment explaining the purpose and usage of the
TextareaPropsinterface. This can enhance maintainability and help new contributors quickly understand its role.+/** + * Defines the props for the Textarea component, extending native HTML textarea attributes. + * Use this interface to pass custom properties to the Textarea component. + */
12-15: Optional: Allow resizing or enforce no-resize policyCurrently, the textarea is styled without explicit mention of its resizing behavior. Consider clarifying whether users should be able to resize the textarea. For instance, adding
resize-nonecan prevent resizing orresize-ycan allow vertical resizing.src/components/Patient/ShiftCreate1.tsx (4)
46-51: Consider adding inline documentation for props.Adding JSDoc or inline comments to clarify the responsibility of each prop (
facilityId,patientId,open,setOpen) would make the code more maintainable and easier to onboard new contributors.
61-100: Unify phone number validations for consistency.Both the patient contact number and ambulance phone number are validated as a 10-digit numeric string and processed by
parsePhoneNumber. Consider extracting this logic into a shared validator or reusing a single schema pattern for phone fields to ensure consistent validation and error messaging.
311-349: Use boolean for checkboxes if feasible.Currently, the “emergency” and “is_up_shift” fields are stored as strings ("true"/"false"). Consider using boolean types in your schema and transforming those values to booleans, which can simplify handling these fields and prevent accidental string comparisons.
528-534: Use a consistent approach for cancellation.The “Cancel” button uses
goBack(). Consider whether you’d prefer to close the slide-over without changing the browser history. Otherwise, for uniform UX, you might want to consistently preserve or discard route history across modals and slide-overs.src/components/ui/form.tsx (3)
17-18: Consider renaming for clarity.
While settingconst Form = FormProvider;is valid, it can be more explicit to rename it to something likeFormProviderWrapperor export it directly with a descriptive name. This can prevent subtle confusion between this object and typical form components.
43-64: Validate the fallback context check logic.
useFormFieldthrows an error iffieldContextis falsy. However,React.createContext<FormFieldContextValue>({} as FormFieldContextValue)provides a non-null default, making it harder forfieldContextto become falsy. Consider initializing the context withnullto ensure your error condition triggers correctly if used outside a provider.
88-110: Accessibility note for required fields.
The current approach appends an asterisk for required fields using pseudo-element styling (after:content). Screen readers typically handle this well, but consider explicitly labeling fields as required in the aria attributes for improved accessibility.src/components/Form/FormFields/PhoneNumberFormField.tsx (2)
46-51: Consider placing helper functions outside component scope.
getStringValueis a handy utility. If it’s used elsewhere, moving it into a shared util file may improve discoverability and reusability across other form fields.
107-118: Edge cases for special prefixes.
Support for1800and+ensures accurate region handling for phone numbers. Keep in mind any future expansions for other special numbers or short codes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.json(2 hunks)src/components/Form/FormFields/PhoneNumberFormField.tsx(4 hunks)src/components/Patient/PatientDetailsTab/ShiftingHistory.tsx(3 hunks)src/components/Patient/ShiftCreate1.tsx(1 hunks)src/components/ui/checkbox.tsx(1 hunks)src/components/ui/form.tsx(1 hunks)src/components/ui/textarea.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Patient/PatientDetailsTab/ShiftingHistory.tsx
🔇 Additional comments (10)
src/components/ui/textarea.tsx (2)
22-22: Well-defined display name
Defining Textarea.displayName helps in debugging and usage within React DevTools. Great job!
8-21: Ensure accessibility compliance
While the forward reference approach and spread props help with flexibility, consider verifying usage of labels, aria-label, or additional descriptive properties wherever this Textarea is used to ensure proper accessibility for screen readers and assistive technologies.
✅ Verification successful
Let me gather more information about the context where these textareas are used.
Accessibility is properly implemented in the Textarea component usage
The verification shows that the Textarea component is used within a proper form structure with appropriate accessibility features:
- Both textarea instances are wrapped in
FormItemcomponents with associatedFormLabelcomponents - The textareas are part of React Hook Form's
FormFieldcomponents which handle form control and validation - Clear, descriptive labels are provided:
- "Any other comments" for the comments field
- "Reason for shift" for the reason field
- Proper placeholder text is included for additional context
The component is being used in an accessible manner within the form structure, with proper labeling and form control integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all usage of `Textarea` and check if relevant accessibility attributes or labels are applied.
rg -A 3 'Textarea' src/
Length of output: 1632
Script:
#!/bin/bash
# Check the form structure and labels around the Textarea usage
cat src/components/Patient/ShiftCreate1.tsx
Length of output: 19425
src/components/Patient/ShiftCreate1.tsx (1)
202-210: Validate slide-over cancellation logic.
Consider whether a user canceling the slide-over should remain on the current page rather than calling goBack(). If the user navigated directly to this page (rather than from a previous route), goBack() may lead to an unintended page or possibly do nothing.
src/components/ui/form.tsx (3)
30-41: Good separation of context and controller.
Wrapping the <Controller> in a context is a neat way to ensure child components can access the field name. This approach is well-structured and preserves the form's internal logic. No issues found here.
111-133: Robust accessibility approach.
Using aria-describedby and aria-invalid is a best practice for accessible UI. This ensures screen readers can properly convey the error and description to users. No changes needed. Great job!
154-179: Dynamic error messages.
FormMessage conditionally rendering the error message is a good approach. Ensure you have corresponding test coverage to verify that errors display appropriately when the field validation fails.
src/components/ui/checkbox.tsx (1)
11-25: Solid implementation with Radix UI.
This checkbox correctly leverages Radix UI’s accessibility features and sets relevant states for disabled, checked, etc. The usage of cn to apply class names based on states is an effective approach.
package.json (1)
59-62: Verify unused dependencies before merging.
New dependencies (@hookform/resolvers, @radix-ui/react-checkbox, and react-hook-form) seem aligned with the changes in the UI components. Confirm that all are actually used in the codebase and that no extra dependencies have inadvertently appeared.
Also applies to: 100-100
✅ Verification successful
All new dependencies are actively used in the codebase
The verification confirms that all newly added dependencies are properly utilized:
@hookform/resolversis used insrc/components/Patient/ShiftCreate1.tsxfor form validation with Zod@radix-ui/react-checkboxis used insrc/components/ui/checkbox.tsxfor the checkbox componentreact-hook-formis used in bothsrc/components/ui/form.tsxandsrc/components/Patient/ShiftCreate1.tsxfor form handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Checks usage of the newly added dependencies across the codebase
rg '@hookform/resolvers'
rg '@radix-ui/react-checkbox'
rg 'react-hook-form'
Length of output: 1793
src/components/Form/FormFields/PhoneNumberFormField.tsx (2)
104-105: Clear pattern for resolving value.
Using resolvedValue consistently simplifies phone number handling and keeps logic contained. Good approach to reduce confusion with possible nested object values.
Line range hint 138-161: Practical max length constraints.
Limiting length dynamically based on resolvedValue is helpful. However, consider whether 15 characters is sufficient for all international formats, especially if the code might expand to more countries in the future.
Also applies to: 163-166
| <FormField | ||
| control={form.control} | ||
| name="ambulance_driver_name" | ||
| render={({ field }) => ( | ||
| <FormItem className="flex flex-col"> | ||
| <FormLabel required>Name of ambulance driver</FormLabel> | ||
| <FormControl> | ||
| <Input | ||
| {...field} | ||
| placeholder="Name of ambulance driver" | ||
| /> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Conditionally require ambulance driver fields.
Not all shifts may require an ambulance driver. If the user opts for a transport that does not involve an ambulance, requiring “ambulance driver name” and phone number may be unnecessary. Consider adding a condition that mandates these fields only when an ambulance is selected.
| const handleSubmit = async (data: ShiftFormData) => { | ||
| setIsLoading(true); | ||
|
|
||
| const payload = { | ||
| ...data, | ||
| status: careConfig.wartimeShifting ? "PENDING" : "APPROVED", | ||
| origin_facility: props.facilityId, | ||
| shifting_approving_facility: | ||
| data.shifting_approving_facility?.id || undefined, | ||
| assigned_facility: data?.assigned_facility?.id || undefined, | ||
| assigned_facility_external: !data?.assigned_facility?.id | ||
| ? data?.assigned_facility?.name || undefined | ||
| : undefined, | ||
| patient: props.patientId, | ||
| emergency: data.emergency === "true", | ||
| is_up_shift: data.is_up_shift === "true", | ||
| reason: data.reason, | ||
| vehicle_preference: data.vehicle_preference, | ||
| comments: data.comments, | ||
| assigned_facility_type: data.assigned_facility_type || undefined, | ||
| preferred_vehicle_choice: data.preferred_vehicle_choice || undefined, | ||
| refering_facility_contact_name: data.refering_facility_contact_name, | ||
| refering_facility_contact_number: parsePhoneNumber( | ||
| data.refering_facility_contact_number, | ||
| ), | ||
| breathlessness_level: data.breathlessness_level, | ||
| patient_category: patientCategory, | ||
| ambulance_driver_name: data.ambulance_driver_name, | ||
| ambulance_phone_number: parsePhoneNumber( | ||
| data.ambulance_phone_number || "", | ||
| ), | ||
| ambulance_number: data.ambulance_number, | ||
| }; | ||
|
|
||
| await request(routes.createShift, { | ||
| body: payload, | ||
| onResponse: ({ res, data }) => { | ||
| setIsLoading(false); | ||
| if (res?.ok && data) { | ||
| form.reset(); | ||
| Notification.Success({ msg: "Shift request created successfully" }); | ||
| goBack(`/shifting/${data.id}`); | ||
| } | ||
| }, | ||
| }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Add error handling for failed requests.
Currently, the code sets isLoading(false) only on the onResponse callback. If the request fails (network/offline errors, server errors), there is no .catch(...) handler to manage or display an error message. To improve reliability, catch these errors and display an appropriate notification.
Proposed approach:
await request(routes.createShift, {
body: payload,
onResponse: ({ res, data }) => {
setIsLoading(false);
if (res?.ok && data) {
form.reset();
Notification.Success({ msg: "Shift request created successfully" });
goBack(`/shifting/${data.id}`);
}
}
+ }).catch((error) => {
+ setIsLoading(false);
+ Notification.Error({ msg: "Shift request failed. Please try again." });
+ console.error("Shift creation error:", error);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleSubmit = async (data: ShiftFormData) => { | |
| setIsLoading(true); | |
| const payload = { | |
| ...data, | |
| status: careConfig.wartimeShifting ? "PENDING" : "APPROVED", | |
| origin_facility: props.facilityId, | |
| shifting_approving_facility: | |
| data.shifting_approving_facility?.id || undefined, | |
| assigned_facility: data?.assigned_facility?.id || undefined, | |
| assigned_facility_external: !data?.assigned_facility?.id | |
| ? data?.assigned_facility?.name || undefined | |
| : undefined, | |
| patient: props.patientId, | |
| emergency: data.emergency === "true", | |
| is_up_shift: data.is_up_shift === "true", | |
| reason: data.reason, | |
| vehicle_preference: data.vehicle_preference, | |
| comments: data.comments, | |
| assigned_facility_type: data.assigned_facility_type || undefined, | |
| preferred_vehicle_choice: data.preferred_vehicle_choice || undefined, | |
| refering_facility_contact_name: data.refering_facility_contact_name, | |
| refering_facility_contact_number: parsePhoneNumber( | |
| data.refering_facility_contact_number, | |
| ), | |
| breathlessness_level: data.breathlessness_level, | |
| patient_category: patientCategory, | |
| ambulance_driver_name: data.ambulance_driver_name, | |
| ambulance_phone_number: parsePhoneNumber( | |
| data.ambulance_phone_number || "", | |
| ), | |
| ambulance_number: data.ambulance_number, | |
| }; | |
| await request(routes.createShift, { | |
| body: payload, | |
| onResponse: ({ res, data }) => { | |
| setIsLoading(false); | |
| if (res?.ok && data) { | |
| form.reset(); | |
| Notification.Success({ msg: "Shift request created successfully" }); | |
| goBack(`/shifting/${data.id}`); | |
| } | |
| }, | |
| }); | |
| }; | |
| const handleSubmit = async (data: ShiftFormData) => { | |
| setIsLoading(true); | |
| const payload = { | |
| ...data, | |
| status: careConfig.wartimeShifting ? "PENDING" : "APPROVED", | |
| origin_facility: props.facilityId, | |
| shifting_approving_facility: | |
| data.shifting_approving_facility?.id || undefined, | |
| assigned_facility: data?.assigned_facility?.id || undefined, | |
| assigned_facility_external: !data?.assigned_facility?.id | |
| ? data?.assigned_facility?.name || undefined | |
| : undefined, | |
| patient: props.patientId, | |
| emergency: data.emergency === "true", | |
| is_up_shift: data.is_up_shift === "true", | |
| reason: data.reason, | |
| vehicle_preference: data.vehicle_preference, | |
| comments: data.comments, | |
| assigned_facility_type: data.assigned_facility_type || undefined, | |
| preferred_vehicle_choice: data.preferred_vehicle_choice || undefined, | |
| refering_facility_contact_name: data.refering_facility_contact_name, | |
| refering_facility_contact_number: parsePhoneNumber( | |
| data.refering_facility_contact_number, | |
| ), | |
| breathlessness_level: data.breathlessness_level, | |
| patient_category: patientCategory, | |
| ambulance_driver_name: data.ambulance_driver_name, | |
| ambulance_phone_number: parsePhoneNumber( | |
| data.ambulance_phone_number || "", | |
| ), | |
| ambulance_number: data.ambulance_number, | |
| }; | |
| await request(routes.createShift, { | |
| body: payload, | |
| onResponse: ({ res, data }) => { | |
| setIsLoading(false); | |
| if (res?.ok && data) { | |
| form.reset(); | |
| Notification.Success({ msg: "Shift request created successfully" }); | |
| goBack(`/shifting/${data.id}`); | |
| } | |
| } | |
| }).catch((error) => { | |
| setIsLoading(false); | |
| Notification.Error({ msg: "Shift request failed. Please try again." }); | |
| console.error("Shift creation error:", error); | |
| }); | |
| }; |
|
@Jacobjeevan is shifting removed from care 🤔 ? Becoz all files related to shifting are missing in the form-fields branch . |
|
👋 Hi, @Mahendar0701, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
It's being removed. |
Proposed Changes
Proposed design : https://www.figma.com/design/Z93EYKSa1MdBmXndsMBJSQ/Care?node-id=3648-1593&p=f&t=74l8V0bymOcWvqdg-0
Shift Patient tab
Create Shift form as slideover
Update Shift form as slide over
List view in shifting
Shift Details Page
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
ReferralLettercomponent for generating patient referral letters.ShiftingTablecomponent with a dropdown menu for shift actions and improved status indicators.Form,FormField, andTextarea.Bug Fixes
Style
SlideOvercomponent and other UI elements.Chores