Add base for fhir forms #9262
Add base for fhir forms #9262
Conversation
![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 (6)
src/components/Schedule/routes.tsx (3)
13-15: Track technical debt: Remove temporary hack.The TODO comment indicates this is a temporary solution pending the facility switcher merge. This should be tracked and addressed once the facility switcher is available.
Would you like me to create a GitHub issue to track this technical debt?
16-37: Enhance error handling and type safety.Consider these improvements:
- The error message should guide users on how to get linked to a home facility.
- The type definition could be more strict using
Required<Pick<User, 'home_facility_object'>>.export const HomeFacilityWrapper = ({ children, user, }: { children: ReactNode; - user?: { home_facility_object?: FacilityModel }; + user?: { home_facility_object?: FacilityModel } | null; }) => { const authUser = useAuthUser(); const resolvedUser = user ?? authUser; if (!resolvedUser.home_facility_object) { return ( <ErrorPage forError="CUSTOM_ERROR" title="No Home Facility" - message="You need to be linked to a home facility to use this feature" + message="You need to be linked to a home facility to use this feature. Please contact your administrator to get linked to a facility." /> ); } return children; };
39-65: Consider using type-safe route parameters.The route parameters could benefit from stronger typing to prevent runtime errors.
Consider using a type-safe routing solution:
type RouteParams = { facilityId: string; patientId: string; appointmentId: string; id: string; }; const ScheduleRoutes: Record<string, (params: Partial<RouteParams>) => JSX.Element> = { // ... routes remain the same };src/components/ErrorPages/DefaultErrorPage.tsx (3)
20-24: Consider the implications of closing all notifications on mount.While closing notifications on mount might be desired in some cases, it could potentially dismiss important notifications that are unrelated to the error page. Consider:
- Adding a cleanup function to the useEffect
- Being more selective about which notifications to close
useEffect(() => { Notification.closeAllNotifications(); + return () => { + // Cleanup if needed + }; }, []);
26-42: Consider differentiating error visuals and custom error handling.Two suggestions for improvement:
- The same image ("/images/404.svg") is used for all error types. Consider using distinct images to help users better understand different error scenarios.
- The CUSTOM_ERROR type currently duplicates PAGE_LOAD_ERROR content. Consider making it more flexible to accept custom content or remove it if not needed.
49-67: Consider improving accessibility and performance.A few suggestions:
- Add explicit width and height attributes to the image to prevent layout shifts during loading
- Provide more descriptive alt text instead of using the title
- Consider adding aria-labels for better screen reader support
- <img src={image} alt={title} className="w-full" /> + <img + src={image} + alt={`Error illustration for ${title}`} + width={500} + height={400} + className="w-full" + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ErrorPages/DefaultErrorPage.tsx(1 hunks)src/components/Schedule/routes.tsx(1 hunks)src/components/Users/UserHome.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Users/UserHome.tsx
🔇 Additional comments (6)
src/components/Schedule/routes.tsx (2)
56-64: Unused patientId parameter in route path
The route includes :patientId but it's not used in the component.
39-65: Verify appointment route coverage.
Let's ensure all necessary appointment-related routes are covered.
✅ Verification successful
All appointment routes are properly covered
Based on the verification results:
- The three main appointment routes in
routes.tsxcover all necessary appointment flows:- Main appointments listing (
/appointments) - Appointment creation (
/facility/:facilityId/patient/:id/appointments/create) - Appointment token management (
/facility/:facilityId/patient/:patientId/appointment/:appointmentId/token)
- Main appointments listing (
- These routes align with the functionality shown in
AppointmentsPage.tsxwhich handles appointment listing, status management (booked, checked-in, in-consultation, fulfilled, no-show), and token generation - The routes follow the consistent pattern of using
HomeFacilityWrapperfor facility validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for appointment-related components and verify route coverage
echo "Searching for appointment-related components..."
fd -e tsx -e ts --full-path "src/components/Schedule" | grep -i "appointment"
echo "Searching for appointment-related route usages..."
rg -t tsx -t ts "appointment" -g "!*.test.*" -g "!*.spec.*"
Length of output: 521
Script:
#!/bin/bash
# Search for appointment-related components and their usages
echo "Searching for appointment-related components in Schedule directory..."
rg -t typescript -t javascriptreact -t typescriptreact "appointment" "src/components/Schedule"
echo -e "\nSearching for appointment-related routes in the entire codebase..."
rg -t typescript -t javascriptreact -t typescriptreact "appointment.*route" -i
echo -e "\nChecking component imports and usages..."
ast-grep --pattern 'import { $_ } from "$_"' src/components/Schedule/routes.tsx
Length of output: 978
Script:
#!/bin/bash
# Let's try with simpler grep commands
echo "Searching for appointment-related files..."
find src -type f -name "*.tsx" -o -name "*.ts" | grep -i "appointment"
echo -e "\nChecking content of AppointmentsPage.tsx..."
cat src/components/Schedule/AppointmentsPage.tsx
echo -e "\nChecking for any other route definitions..."
find src -type f -name "*.tsx" -o -name "*.ts" | xargs grep -l "AppRoutes"
Length of output: 12578
src/components/ErrorPages/DefaultErrorPage.tsx (4)
1-14: LGTM! Well-structured type definitions and imports.
The type definitions are clear, properly constrained, and follow TypeScript best practices. The imports are appropriate for the component's needs.
16-19: LGTM! Clean component definition with proper props handling.
The component follows React best practices with appropriate default props and rest operator usage.
44-47: LGTM! Clean implementation of content override mechanism.
The props spreading implementation allows for flexible content customization while maintaining good defaults.
1-68: Verify the relationship with PR objectives.
The changes to this error page component, while well-implemented, seem unrelated to the PR's stated objective of "Add base for fhir forms". Please clarify if this is an intentional part of the FHIR forms implementation or if it should be in a separate PR.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/Utils/request/api.tsx (3)
1337-1394:⚠️ Potential issueAdd validation for questionnaire response values
The
questionnaire.submitendpoint accepts complex response data without explicit validation. To ensure data integrity and prevent potential errors, consider adding runtime validation for the request body. This includes:
- Ensuring required fields (
resource_id,encounter, andresponses) are present.- Validating that each item in
responseshas the correct types forquestion_idandvalue.- Confirming that optional fields (
note,bodysite,method) are properly typed when present.Would you like assistance in implementing validation using a library like
zodoryup?
1409-1409:⚠️ Potential issueFix inconsistent placeholder syntax in route path
The route for
patient.allergyIntolerance.createuses:patientIdas a placeholder, which is inconsistent with the{patientId}syntax used elsewhere. Replace:patientIdwith{patientId}to maintain consistency across the codebase.Apply this diff to fix the issue:
path: "/api/v1/patient/:patientId/allergy_intolerance/", path: "/api/v1/patient/{patientId}/allergy_intolerance/",
1427-1428: 🛠️ Refactor suggestionUse
TBodyinstead ofTReqfor consistencyThroughout the API definitions,
TBodyis used to specify the request body type. IncreatePlugConfigandupdatePlugConfig,TReqis used instead, which is inconsistent. For consistency and clarity, replaceTReqwithTBody.Apply this diff to align with the convention:
// For createPlugConfig createPlugConfig: { path: "/api/v1/plug_config/", method: "POST", - TReq: Type<PlugConfig>(), + TBody: Type<PlugConfig>(), TRes: Type<PlugConfig>(), }, // For updatePlugConfig updatePlugConfig: { path: "/api/v1/plug_config/{slug}/", method: "PATCH", - TReq: Type<PlugConfig>(), + TBody: Type<Partial<PlugConfig>>(), TRes: Type<PlugConfig>(), },src/pages/Patient/PatientRegistration.tsx (4)
76-77: 🛠️ Refactor suggestionEnhance type safety in form validation
The
validateFormfunction uses theanytype, which bypasses TypeScript's type checking benefits. Specify the form parameter type to enable compile-time checks and improve code reliability.Apply this diff to improve type safety:
-const validateForm = (form: any) => { - const errors: Partial<Record<keyof any, FieldError>> = {}; +const validateForm = (form: AppointmentPatientRegister) => { + const errors: Partial<Record<keyof AppointmentPatientRegister, FieldError>> = {};
180-183:⚠️ Potential issueEnsure
phoneNumberis available before submittingIn the
handleSubmitfunction,phoneNumbermight benullorundefined, resulting in an empty string forphone_number. Submitting without a valid phone number may cause backend issues. ValidatephoneNumberand handle cases where it's not available.
279-309:⚠️ Potential issueCorrect event handling in
handlePincodeChangeIn the
handlePincodeChangefunction, accessinge.valueis incorrect becauseeis an event object. You should access the value viae.target.value. Also, adjust the parameters to properly receive the value.Apply this diff to fix event handling:
-const handlePincodeChange = async (e: any, setField: any) => { - if (!validatePincode(e.value)) return; - const pincodeDetails = await getPincodeDetails( - e.value, +const handlePincodeChange = async (value: string, setField: any) => { + if (!validatePincode(value)) return; + const pincodeDetails = await getPincodeDetails( + value, careConfig.govDataApiKey, );Update the
onChangehandler:onChange={(e) => { field("pincode").onChange(e); - handlePincodeChange(e, field("pincode").onChange); + handlePincodeChange(e.target.value, field("pincode").onChange); }}
294-295:⚠️ Potential issueFix condition to check for empty
fetchedDistrictsAn empty array is truthy in JavaScript, so
if (!fetchedDistricts)will not catch empty arrays. Update the condition to check the array length.Apply this diff to correct the condition:
- if (!fetchedDistricts) return; + if (!fetchedDistricts || fetchedDistricts.length === 0) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Utils/request/api.tsx(4 hunks)src/pages/Patient/PatientRegistration.tsx(1 hunks)src/pages/Patient/PatientSelect.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/Patient/PatientSelect.tsx
🔇 Additional comments (4)
src/Utils/request/api.tsx (1)
1324-1329: Confirm intention behind commenting out valueset.list route
The valueset.list route is commented out. If this is intentional due to deprecation or pending implementation, it's acceptable. Otherwise, consider uncommenting or removing it to maintain code clarity.
src/pages/Patient/PatientRegistration.tsx (3)
232-244: Implement error handling in fetchDistricts function
The fetchDistricts function lacks error handling, which could lead to silent failures. Adding try-catch blocks and user notifications will enhance reliability.
Apply this diff to handle errors:
const fetchDistricts = useCallback(async (id: number) => {
if (id > 0) {
setIsDistrictLoading(true);
+ try {
const { res, data } = await request(routes.getDistrictByState, {
pathParams: { id },
});
if (res?.ok && data) {
setDistricts(data);
return data;
}
+ throw new Error("Failed to fetch districts");
+ } catch (error) {
+ Notification.Error({ msg: "Failed to fetch districts. Please try again." });
+ return [];
+ } finally {
setIsDistrictLoading(false);
}
}
+ return [];
}, []);105-112: 🛠️ Refactor suggestion
Add upper age limit in validation
Currently, only negative ages are checked. It's advisable to set a reasonable upper limit to age to prevent invalid data entry, such as ages over 120 years.
Apply this diff to enhance validation:
if (field === "year_of_birth") {
if (form.year_of_birth < 0 || form.year_of_birth > 120) {
errors.year_of_birth = "Age must be between 0 and 120";
return;
}
form.date_of_birth = null;
form.year_of_birth = new Date().getFullYear() - form.year_of_birth;
}Likely invalid or redundant comment.
206-223: 🛠️ Refactor suggestion
Add error handling for patient registration failures
The handleSubmit function does not handle API errors or provide user feedback when the patient registration fails. Adding error handling improves user experience and debuggability.
Apply this diff to include error handling:
const handleSubmit = async (formData: AppointmentPatientRegister) => {
const data = {
// ...existing data fields...
};
+ try {
const response = await fetch(`${careConfig.apiUrl}/api/v1/otp/patient/`, {
method: "POST",
headers: {
Authorization: `Bearer ${OTPaccessToken}`,
"Content-Type": "application/json",
},
body: JSON.stringify(data),
});
const requestData = await response.json();
if (response.ok && requestData) {
publish("patient:upsert", requestData);
createAppointment({
patient: requestData.id,
reason_for_visit: reason ?? "",
});
+ } else {
+ throw new Error(requestData?.detail || "Failed to register patient");
+ }
+ } catch (error) {
+ Notification.Error({
+ msg: error.message || "An unexpected error occurred during patient registration.",
+ });
}
};Likely invalid or redundant comment.
| type HttpMethod = "GET" | "POST" | "PUT" | "PATCH" | "DELETE"; | ||
|
|
||
| export const API = <TResponse = undefined, TBody = undefined>( | ||
| route: `${HttpMethod} ${string}`, | ||
| ) => { | ||
| const [method, path] = route.split(" ") as [HttpMethod, string]; | ||
| return { | ||
| path, | ||
| method, | ||
| TRes: Type<TResponse>(), | ||
| TBody: Type<TBody>(), | ||
| }; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure robust parsing of the route string in API function
The current implementation of route.split(" ") may result in incorrect parsing if there are extra spaces in the route string. This could lead to errors in extracting the HTTP method and path. Consider trimming the route and splitting on one or more whitespace characters to ensure accurate parsing.
Apply this diff to improve robustness:
export const API = <TResponse = undefined, TBody = undefined>(
route: `${HttpMethod} ${string}`,
) => {
- const [method, path] = route.split(" ") as [HttpMethod, string];
+ const [method, path] = route.trim().split(/\s+/) as [HttpMethod, string];
return {
path,
method,
TRes: Type<TResponse>(),
TBody: Type<TBody>(),
};
};📝 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.
| type HttpMethod = "GET" | "POST" | "PUT" | "PATCH" | "DELETE"; | |
| export const API = <TResponse = undefined, TBody = undefined>( | |
| route: `${HttpMethod} ${string}`, | |
| ) => { | |
| const [method, path] = route.split(" ") as [HttpMethod, string]; | |
| return { | |
| path, | |
| method, | |
| TRes: Type<TResponse>(), | |
| TBody: Type<TBody>(), | |
| }; | |
| }; | |
| type HttpMethod = "GET" | "POST" | "PUT" | "PATCH" | "DELETE"; | |
| export const API = <TResponse = undefined, TBody = undefined>( | |
| route: `${HttpMethod} ${string}`, | |
| ) => { | |
| const [method, path] = route.trim().split(/\s+/) as [HttpMethod, string]; | |
| return { | |
| path, | |
| method, | |
| TRes: Type<TResponse>(), | |
| TBody: Type<TBody>(), | |
| }; | |
| }; |
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| phoneNumber: phoneNumber, | ||
| createdAt: new Date().toISOString(), | ||
| }; | ||
| localStorage.setItem(CarePatientTokenKey, JSON.stringify(tokenData)); |
Check failure
Code scanning / CodeQL
Clear text storage of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 8 hours ago
To fix the problem, we need to ensure that the phone number is encrypted before being stored in localStorage. We can use the crypto module from Node.js to encrypt the phone number. The encryption key should be securely managed and not hardcoded in the code.
- Import the
cryptomodule. - Create an encryption function that uses a secure algorithm to encrypt the phone number.
- Encrypt the phone number before storing it in
localStorage. - Decrypt the phone number when it needs to be used.
| @@ -22,2 +22,3 @@ | ||
| import { TokenData } from "@/types/auth/otpToken"; | ||
| import crypto from "crypto"; | ||
|
|
||
| @@ -33,2 +34,13 @@ | ||
| const { goBack } = useAppHistory(); | ||
| const encryptionKey = "your-secure-encryption-key"; // Replace with a secure key | ||
|
|
||
| const encrypt = (text: string) => { | ||
| const cipher = crypto.createCipher("aes-256-ctr", encryptionKey); | ||
| return cipher.update(text, "utf8", "hex") + cipher.final("hex"); | ||
| }; | ||
|
|
||
| const decrypt = (text: string) => { | ||
| const decipher = crypto.createDecipher("aes-256-ctr", encryptionKey); | ||
| return decipher.update(text, "hex", "utf8") + decipher.final("utf8"); | ||
| }; | ||
| const { t } = useTranslation(); | ||
| @@ -98,2 +110,3 @@ | ||
| }; | ||
| tokenData.phoneNumber = encrypt(tokenData.phoneNumber); | ||
| localStorage.setItem(CarePatientTokenKey, JSON.stringify(tokenData)); |
Proposed Changes
TBD
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
EncounterQuestionnaire,QuestionRenderer, and various question types (e.g.,BooleanQuestion,TextQuestion,NumberQuestion).AppointmentCreatePage,AppointmentsPage, andAppointmentTokenPage.Calendarcomponent for date selection and aDatePickerfor date input.FacilitiesPage,FacilityDetailsPage, and various appointment-related routes.SymptomsListandDiagnosisListcomponents to display patient symptoms and diagnoses.ResourceRequestscomponent for managing resource requests related to patients.Callout,ColoredIndicator, andDrawercomponents for improved UI interactions.UserAvailabilityTabfor managing user availability in a calendar format.ScheduleTemplateFormandScheduleTemplatesListfor managing schedule templates.Toastercomponent for improved notification handling.Logincomponent to support staff and patient login modes with improved state management and validation.AllergyListandDiagnosisListcomponents for displaying patient allergies and diagnoses.ScheduleExceptionFormandScheduleExceptionsListfor managing schedule exceptions.AppointmentSuccessandOTPcomponents for appointment confirmation and OTP verification.ColoredIndicatorandCalloutcomponents for enhanced UI feedback.PatientRegistrationandPatientSelectcomponents for improved patient management functionality.FacilityHomeandFacilityDetailsPageto streamline facility management processes.Bug Fixes
Documentation
Chores