Modify CreateUserForm and add dirtyState to all react-hook-form
#9749
Conversation
WalkthroughThe pull request introduces a consistent approach to form handling across multiple components by implementing an Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Users/CreateUserForm.tsx (1)
158-159: Remove or refine console logs for production.
console.log(!!errors, errors);may not be intended for production. It can clutter logs.- console.log(!!errors, errors); + // Remove this before production or replace with a proper logger if needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/Encounter/CreateEncounterForm.tsx(2 hunks)src/components/Facility/FacilityCreate.tsx(2 hunks)src/components/Schedule/ScheduleExceptionForm.tsx(2 hunks)src/components/Users/CreateUserForm.tsx(15 hunks)src/pages/Appoinments/PatientRegistration.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/pages/Appoinments/PatientRegistration.tsx (1)
Learnt from: kihan2518B
PR: ohcnetwork/care_fe#8956
File: src/components/Patient/PatientRegister.tsx:861-861
Timestamp: 2024-12-02T18:50:29.685Z
Learning: In `src/components/Patient/PatientRegister.tsx`, `insuranceDetails` is not part of `state.form`, so changes to it are not tracked by the `isDirty` state. Therefore, the form's `disabled` state needs to be conditionally set based on `insuranceDetails.length`.
🔇 Additional comments (17)
src/components/Schedule/ScheduleExceptionForm.tsx (2)
76-79: Good addition ofisDirtyfor form-checking.
UsingisDirtyclarifies when the user has modified form data, enhancing UX by preventing accidental submissions. Nicely done!
259-263: Great user feedback improvement.
Disabling the submit button when the form is clean or a mutation is pending helps avoid redundant submissions. This aligns well with best practices.src/components/Encounter/CreateEncounterForm.tsx (2)
148-151: Clear adoption ofisDirtytracking.
ExtractingisDirtyfrom the form state neatly sets you up to conditionally enable/disable submission. No issues found here.
325-325: Submission button logic is consistent with design goals.
Disabling the button if the form has not been modified prevents unnecessary submissions. This is a solid improvement.src/components/Users/CreateUserForm.tsx (9)
26-27: NewDateFormFieldimport.
Importing this custom date field is fine. Ensure unit tests cover edge cases (e.g., invalid dates, future dates).Could you confirm if we have an existing suite for DateFormField coverage?
79-82: Schema definitions updated.
Refiningdate_of_birthas az.date()and expanding gender options broadens validation scope. Nicely done!
103-103: Form validation mode switched toonChange.
Real-time validation is generally user-friendly and can catch errors earlier. Looks good.
114-117: Kudos on extracting more form states.
AccessingisDirty,errors, andisValidbroadens control over user feedback.
194-194: Clear required field labeling.
Adding therequiredindicator improves clarity for end-users. Nice enhancement.Also applies to: 208-208, 223-223, 238-238, 252-252, 271-271
286-293: Phone number length limit.
SettingmaxLength={13}helps ensure consistent formatting. This is aligned with the requirement for +91 and 10 digits.
305-305: Renaming alt phone to WhatsApp is clear.
Enforcing the same max length ensures uniform input constraints. Good job.Also applies to: 311-311
345-345: Use ofdisableFutureis user-friendly.
Disabling future dates adds appropriate domain constraints. Nicely implemented.Also applies to: 347-352
462-466: Conditionally disabling submission.
Ensuring the form is both dirty and valid before submission is a strong UX improvement.src/pages/Appoinments/PatientRegistration.tsx (2)
142-145:isDirtyextraction is consistent with PR objectives.
This approach prevents registering unchanged data. No issues found.
455-455: Disable submit button when the form is clean.
Mirrors similar logic across the application, ensuring consistent user flow. Nicely aligned with other forms.src/components/Facility/FacilityCreate.tsx (2)
140-143: LGTM! Clean implementation of form state tracking.The destructuring of
isDirtyfromformStatefollows react-hook-form best practices and properly implements form modification tracking.
Line range hint
147-164: Verify form reset behavior with isDirty state.The form reset after loading facility data might affect the dirty state tracking. Let's verify this behavior to ensure it doesn't cause unexpected issues.
✅ Verification successful
Form reset behavior is correctly implemented
The form reset behavior in FacilityCreate is properly implemented and doesn't cause any issues with the dirty state tracking:
- The component correctly tracks the form's dirty state using
formState: { isDirty }from the form hook- The form reset is used appropriately in the
useEffectwhen loading facility data- The submit button is properly disabled when the form is not dirty:
disabled={isLoading || !isDirty}- The form initialization includes proper default values, ensuring consistent state tracking
The current implementation follows the recommended pattern for form state management in React Hook Form, and the dirty state will accurately reflect whether the user has made changes to the form values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar patterns in other form components to understand the reset behavior ast-grep --pattern 'form.reset($$$)' # Look for any related issues or discussions rg -g '!node_modules' -l 'isDirty.*form\.reset|form\.reset.*isDirty'Length of output: 1481
Script:
#!/bin/bash # Let's check the form hook initialization and usage in FacilityCreate ast-grep --pattern 'const form = useForm<$_>($$$)' # Let's also check for any isDirty usage in the component rg -A 5 'isDirty' src/components/Facility/FacilityCreate.tsx # Check the form submission handler to understand the form state management ast-grep --pattern 'const onSubmit = async ($$$) => { $$$ }'Length of output: 11138
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Users/CreateUserForm.tsx (2)
300-305: Consider improving the phone number placeholder.While the maxLength implementation is good, the placeholder could be more descriptive to match the validation requirements.
- placeholder="+91XXXXXXXXXX" + placeholder="+91 followed by 10 digits"Also applies to: 317-319, 325-325
363-368: Consider adding error handling for date changes.While the DateFormField implementation looks good, consider adding error handling for the onChange callback.
- onChange={(date) => field.onChange(date.value)} + onChange={(date) => { + if (date?.value) { + field.onChange(date.value); + } + }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/Encounter/CreateEncounterForm.tsx(1 hunks)src/components/Facility/FacilityCreate.tsx(1 hunks)src/components/Schedule/ScheduleExceptionForm.tsx(1 hunks)src/components/Users/CreateUserForm.tsx(13 hunks)src/pages/Appoinments/PatientRegistration.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pages/Appoinments/PatientRegistration.tsx
- src/components/Encounter/CreateEncounterForm.tsx
- src/components/Schedule/ScheduleExceptionForm.tsx
- src/components/Facility/FacilityCreate.tsx
🔇 Additional comments (2)
src/components/Users/CreateUserForm.tsx (2)
79-86: LGTM! Schema changes look good.The date validation and gender options have been improved:
- Date of birth validation now properly prevents future dates
- Gender options are more inclusive with the addition of "transgender" and "non_binary"
107-107: LGTM! Form configuration enhances UX.The form configuration improvements align with the PR objectives:
- Real-time validation with "onChange" mode
- Submit button properly disabled until form is both dirty and valid
Also applies to: 481-481
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Users/CreateUserForm.tsx (1)
475-479: Consider adding a loading state to the submit buttonThe form validation using both
isDirtyandisValidis excellent. However, to prevent double submissions and provide better feedback, consider adding a loading state to the button during form submission.<Button type="submit" className="w-full" - disabled={!form.formState.isDirty || !form.formState.isValid} + disabled={!form.formState.isDirty || !form.formState.isValid || form.formState.isSubmitting} > - Create User + {form.formState.isSubmitting ? "Creating..." : "Create User"} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/CreateUserForm.tsx(13 hunks)
🔇 Additional comments (5)
src/components/Users/CreateUserForm.tsx (5)
9-9: Good choice using shadcn's DatePicker!The switch to shadcn's DatePicker component aligns with our component standardization efforts.
78-85: Well-implemented schema validation!The date_of_birth validation is thorough with:
- Required field validation
- Future date restriction
- Proper error messages
The gender options are now more inclusive with the addition of "transgender" and "non_binary".
106-106: Good implementation of real-time validation!Setting form mode to "onChange" enables immediate feedback as users type, improving the user experience.
299-304: Good input constraints for phone number!The maxLength attribute ensures phone numbers don't exceed 13 characters (including the +91 prefix).
362-365: Clean implementation of the date picker!The DatePicker implementation is concise and properly handles the date value and onChange events.
|
Everything is working fine but |
|
Check the network response, you will be able top see the error message. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/components/ui/date-picker.tsx (2)
19-19: Clarify prop naming for better readability.The
disabledprop name may be ambiguous since it's often associated with a boolean property that disables the entire component. Consider renaming this callback to something likedisableDateorisDateDisabledto more clearly convey its purpose.
22-22: Ensure function defaults are handled properly.If consumers do not pass the
disabledprop, ensure you have a suitable fallback in place so theCalendarcomponent won't receiveundefined. You can default to a no-op function (always returningfalse) for a cleaner developer experience.src/components/Users/CreateUserForm.tsx (1)
295-297: Consider clarifying phone input design.You’ve limited the field to 13 characters (
+91+ 10 digits). This works if phone numbers will always begin with "+91". For broader usage or future expansions beyond Indian country codes, you might need a separate strategy or dynamic length checks.Also applies to: 299-304
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Users/CreateUserForm.tsx(13 hunks)src/components/ui/date-picker.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: cypress-run (1)
- GitHub Check: CodeQL-Build
- GitHub Check: OSSAR-Scan
🔇 Additional comments (6)
src/components/ui/date-picker.tsx (1)
48-48: Verify that theCalendarcomponent accepts a function for thedisabledprop.Ensure the
Calendarcomponent's interface expects a function for thedisabledprop, as documented. Otherwise, it might cause unintended behavior or runtime errors.✅ Verification successful
The
disabledprop is correctly used with the Calendar componentLooking at the implementation in
src/components/ui/calendar.tsx, the Calendar component is a wrapper around the DayPicker component and forwards all props (includingdisabled) through the spread operator{...props}. The component accepts all DayPicker props, which includes thedisabledprop that can be a function, date, or array of dates to disable specific days.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Calendar component signature for the 'disabled' prop rg -A 10 "function Calendar" --type ts --type tsxLength of output: 79
Script:
#!/bin/bash # Search for Calendar component definition and its props interface/type rg -t ts -t typescript -t tsx "Calendar.*Props|interface.*Calendar|type.*Calendar|function Calendar|const Calendar" -A 10 # Also search for any re-exports of Calendar component rg -t ts -t typescript -t tsx "export.*Calendar"Length of output: 244
Script:
#!/bin/bash # Let's try with fd to find relevant files first fd -e ts -e tsx # Then use ast-grep to find Calendar component definition and props ast-grep --pattern 'interface CalendarProps { $$$ }' ast-grep --pattern 'type CalendarProps = { $$$ }' ast-grep --pattern 'const Calendar = $$$' ast-grep --pattern 'function Calendar($$$) { $$$ }'Length of output: 29547
src/components/Users/CreateUserForm.tsx (5)
78-85: Confirm date comparison logic.The Zod refinement checks that
dob <= new Date(), which is correct to prevent future dates. However, confirm that all needed time zones or edge cases (e.g., local midnight offsets) are handled. For critical date validations, consider normalizing times (e.g., to midnight) prior to comparison.
106-106: Great use of onChange validation mode.This aligns well with real-time user feedback, improving user experience.
191-193: Consistent required field indicators.Appending “*” clarifies which fields are mandatory. The approach is clear and user-friendly, ensuring consistent labeling across the form.
Also applies to: 207-209, 224-226, 241-243, 257-259, 278-280
316-318: WhatsApp number approach.Disabling the field when
isWhatsAppistrueis intuitive. If users later uncheck “WhatsApp number is same as phone number,” they can provide a different number. Just ensure you handle edge cases when togglingisWhatsAppon/off.Also applies to: 324-324
476-480: Great synergy between isDirty and isValid checks.Disabling the submit button if the form is either pristine or invalid ensures fewer erroneous or empty submissions. This usage is consistent with best practices for react-hook-form. Nicely done.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Users/CreateUserForm.tsx (2)
299-304: Consider extracting the phone number maxLength as a constant.The maxLength of 13 (accounting for "+91" prefix and 10 digits) is used in multiple places. Consider extracting it as a named constant for better maintainability and clarity.
+ const PHONE_NUMBER_MAX_LENGTH = 13; // +91 prefix (3) + 10 digits ... <Input type="tel" placeholder="+91XXXXXXXXXX" - maxLength={13} + maxLength={PHONE_NUMBER_MAX_LENGTH} {...field} />Also applies to: 324-324
362-368: Consider extracting date constraints as named constants.The date restrictions use magic values that could be more maintainable and self-documenting as constants.
+ const MIN_DATE_OF_BIRTH = new Date("1900-01-01"); + // Prevent future dates and dates before 1900 + const isDateDisabled = (date: Date) => + date > new Date() || date < MIN_DATE_OF_BIRTH; ... <DatePicker date={field.value} onChange={(date) => field.onChange(date)} - disabled={(date) => - date > new Date() || date < new Date("1900-01-01") - } + disabled={isDateDisabled} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/CreateUserForm.tsx(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/components/Users/CreateUserForm.tsx (4)
9-9: LGTM! Using the recommended DatePicker component.The change follows the recommendation to use shadcn's datepicker instead of the deprecated DateFormField component.
78-85: LGTM! Comprehensive validation for date_of_birth and inclusive gender options.The schema changes include:
- Strong typing and validation for date_of_birth
- Prevention of future dates
- More inclusive gender options including "transgender" and "non_binary"
106-106: LGTM! Real-time validation enabled.Setting form mode to "onChange" enables real-time error tracking as users input data, improving user experience.
478-482: LGTM! Robust form submission control.The submit button implementation:
- Prevents submission of unmodified forms (
isDirty)- Ensures form validity before submission (
isValid)- Reduces unnecessary network requests
This implementation should be considered as the standard pattern for other forms in the application.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Users/CreateUserForm.tsx (1)
147-157: Consider optimizing error notificationsWhile the error handling is thorough, showing both form field errors and notifications for the same errors might be redundant. Consider showing notifications only for general errors and letting form field errors handle field-specific validation failures.
if (error) { const errors = (error.errors as any[]) || []; errors.forEach((err) => { const field = err.loc[0]; form.setError(field, { message: err.msg }); - Notification.Error({ - msg: err.msg, - }); }); + if (errors.length > 0) { + Notification.Error({ + msg: "Please correct the highlighted fields", + }); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/CreateUserForm.tsx(14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (5)
src/components/Users/CreateUserForm.tsx (5)
78-85: Well-implemented schema changes!The date of birth validation and expanded gender options are well thought out. The validation ensures dates are not in the future while the gender options are more inclusive.
106-106: Good choice of form validation mode!Setting mode to "onChange" provides real-time feedback to users, enhancing the user experience by showing validation errors as they type.
307-312: Good input constraints for phone number!The maxLength attribute ensures phone numbers don't exceed 13 characters (including the +91 prefix), which aligns with the validation requirements.
370-376: Well-implemented date picker constraints!The date picker properly restricts both future dates and dates before 1900, providing a good user experience while ensuring valid data entry.
486-490: Excellent form submission control!The submit button implementation correctly uses both
isDirtyandisValidstates, preventing unnecessary API calls and ensuring data validity before submission. This matches the standardization suggested in previous reviews.
got it, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Patient/PatientRegistration.tsx (1)
522-531: Consider adding a maximum allowable age validation.While this logic prevents negative ages, extremely large values may yield unrealistic birth years. Restricting the age range (e.g., 0-120) or dynamically validating the resulting
year_of_birthcould provide a more robust user experience.
|
👋 Hi, @rajku-dev, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Users/CreateUserForm.tsx (4)
67-69: Consider making phone number validation more flexibleThe current regex
/^\+91[0-9]{10}$/only allows Indian phone numbers. Consider making it more flexible for international numbers if the system needs to support users from different countries.- phone_number: z - .string() - .regex(/^\+91[0-9]{10}$/, t("phone_number_must_start")), + phone_number: z + .string() + .regex(/^\+[1-9]\d{1,14}$/, t("invalid_phone_number")),
75-81: Enhance date of birth validationWhile the future date validation is good, consider adding a reasonable minimum date validation in the schema to match the UI restriction.
date_of_birth: z .date({ required_error: t("this_field_is_required"), }) - .refine((dob) => dob <= new Date(), { - message: t("date_of_birth_cannot_be_in_future"), + .refine((dob) => { + const minDate = new Date("1900-01-01"); + const today = new Date(); + return dob >= minDate && dob <= today; + }, { + message: t("invalid_date_of_birth_range"), }),
124-129: Improve type safety in error handlingThe current error handling uses type assertion (
as any[]). Consider defining proper error types for better type safety.+ type ApiError = { + loc: string[]; + ctx: { error: string }; + }; - const errors = (error.cause?.errors as any[]) || []; + const errors = (error.cause?.errors as ApiError[]) || [];
266-271: Enhance phone number input validationWhile
maxLengthhelps, consider adding thepatternattribute to provide better input validation feedback in supported browsers.<Input type="tel" placeholder="+91XXXXXXXXXX" maxLength={13} + pattern="^\+[0-9]{2}[0-9]{10}$" {...field} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Users/CreateUserForm.tsx(13 hunks)src/types/user/userApi.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types/user/userApi.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/components/Users/CreateUserForm.tsx (1)
441-449: LGTM! Well-implemented submit buttonThe submit button implementation follows best practices:
- Properly handles form state (
isDirtyandisValid)- Shows loading indicator during submission
- Uses translations for button text
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Users/CreateUserForm.tsx (3)
44-91: Consider enhancing validation rules.The form schema is well-structured, but consider these improvements:
- The phone number regex (
/^\+91[0-9]{10}$/) only allows Indian numbers. Consider making it more flexible for international numbers if needed.- The date validation could be more specific about the minimum allowed date.
115-128: Improve type safety in error handling.The error handling could be more type-safe:
- const errors = (error.cause?.errors as any[]) || []; + type ApiError = { loc: string[]; ctx: { error: string } }; + const errors = (error.cause?.errors as ApiError[]) || [];
264-269: Enhance phone number input accessibility.Consider adding the
patternattribute to improve mobile keyboard experience:<Input type="tel" placeholder="+91XXXXXXXXXX" maxLength={13} + pattern="[+][0-9]{12}" {...field} />Also applies to: 287-289
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
public/locale/en.json(4 hunks)src/components/Encounter/CreateEncounterForm.tsx(1 hunks)src/components/Users/CreateUserForm.tsx(13 hunks)src/components/Users/UserFormValidations.tsx(2 hunks)src/pages/Appoinments/PatientRegistration.tsx(1 hunks)src/types/user/user.ts(1 hunks)src/types/user/userApi.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pages/Appoinments/PatientRegistration.tsx
- src/components/Encounter/CreateEncounterForm.tsx
- src/types/user/userApi.ts
- public/locale/en.json
🔇 Additional comments (4)
src/types/user/user.ts (1)
18-35: LGTM! Well-structured type definition.The
UserCreateRequesttype properly captures all required fields for user creation with appropriate optionality and type constraints.src/components/Users/UserFormValidations.tsx (1)
Line range hint
10-41: LGTM! Well-organized field groupings.The field arrays are properly updated to use the new
UserCreateRequesttype and maintain logical groupings for different operations.src/components/Users/CreateUserForm.tsx (2)
323-329: LGTM! Proper date constraints implemented.The DatePicker implementation correctly handles date constraints and value management.
439-448: LGTM! Proper form submission handling.The submit button correctly implements:
- Form state validation (
isDirtyandisValid)- Loading state
- Translations
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Users/UserFormValidations.tsx (1)
Missing required field in newUserFields array
The
newUserFieldsarray is missing the requireddate_of_birthfield fromUserCreateRequest. The fields mentioned in the review comment (c_passwordandphone_number_is_whatsapp) are not part of the type definition, so they should not be included.
- Add
date_of_birthto thenewUserFieldsarray insrc/components/Users/UserFormValidations.tsx🔗 Analysis chain
Line range hint
10-19: Verify field arrays match the schemaThe field arrays have been updated to use
UserCreateRequest, but some fields likec_passwordandphone_number_is_whatsappare included in the schema but not in these arrays. Consider updating the arrays to match the schema exactly.Also applies to: 22-27, 30-33, 36-39
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if all fields in userFormSchema are included in the field arrays # Search for the schema definition echo "Schema fields:" rg "userFormSchema.*object.*\{" -A 50 "src/components/Users/CreateUserForm.tsx" # Compare with field arrays echo -e "\nField arrays:" rg "Array<keyof UserCreateRequest>" -A 10 "src/components/Users/UserFormValidations.tsx"Length of output: 954
Script:
#!/bin/bash # Try different patterns to locate schema and type definitions # Search for schema definitions with broader patterns echo "Searching for schema definitions:" rg "schema.*=.*{|type.*Schema.*=.*{" -A 20 "src/components/Users/CreateUserForm.tsx" # Search for UserCreateRequest type definition echo -e "\nSearching for UserCreateRequest type:" ast-grep --pattern 'type UserCreateRequest = { $$$ }' # Alternative: search for interface ast-grep --pattern 'interface UserCreateRequest { $$$ }'Length of output: 1341
🧹 Nitpick comments (2)
src/components/Users/CreateUserForm.tsx (2)
93-94: Consider optimizing error handlingThe mutation error handling assumes a specific error structure. Consider:
- Adding type safety for the error structure
- Adding a fallback error message for unexpected error formats
onError: (error) => { + if (!error.cause?.errors?.length) { + toast.error(t("something_went_wrong")); + return; + } const errors = (error.cause?.errors as any[]) || []; errors.forEach((err) => { const field = err.loc[0]; form.setError(field, { message: err.ctx.error }); }); },Also applies to: 115-128
130-134: Simplify object destructuringThe current approach using an IIFE for object destructuring could be simplified.
- const onSubmit = (data: z.infer<typeof userFormSchema>) => - createUser( - (({ c_password: _c, phone_number_is_whatsapp: _p, ...payload }) => - payload)(data), - ); + const onSubmit = ({ c_password, phone_number_is_whatsapp, ...payload }: z.infer<typeof userFormSchema>) => + createUser(payload);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Users/CreateUserForm.tsx(13 hunks)src/components/Users/UserFormValidations.tsx(2 hunks)src/types/user/user.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types/user/user.ts
🔇 Additional comments (4)
src/components/Users/CreateUserForm.tsx (4)
44-91: Schema validation looks robustThe schema includes comprehensive validation rules with proper error messages using translations. Good job on:
- Username validation with regex patterns
- Password complexity requirements
- Phone number format validation
- Date of birth validation
263-268: Good phone number input constraintsThe maxLength attribute on phone number inputs helps prevent invalid input.
Also applies to: 286-286
322-328: Date picker implementation looks goodThe date picker correctly:
- Prevents selection of future dates
- Sets a reasonable minimum date
- Uses the new shadcn DatePicker component as recommended
438-446: Submit button implementation follows best practicesThe button correctly:
- Disables submission when form is invalid or unchanged
- Shows loading state during submission
- Uses translations for the button text
| createUser( | ||
| (({ c_password: _c, phone_number_is_whatsapp: _p, ...payload }) => | ||
| payload)(data), | ||
| ); |
There was a problem hiding this comment.
this is not clean. let's not do weird things to achieve what's desired. let's maintain only readable code.
let's pick things only what we need manually here. so that later if we ever change the form for some reason (lets say we added a new field that doesn't needs to be sent to the backend, this would end up sending information that backend doesn't care about).
follow good patterns that are done in other places in the codebase that uses onSubmit with useForm.
createUser({
username: data.username,
first_name: data.first_name,
...
})
There was a problem hiding this comment.
used transform method to prevent sending empty strings to backend for optional field
could've also done like
qualification: data.qualification || undefined # for each optional fieldworks same.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/CreateUserForm.tsx(13 hunks)
🔇 Additional comments (5)
src/components/Users/CreateUserForm.tsx (5)
44-104: LGTM! Well-structured schema with comprehensive validation rules.The schema implementation is thorough with:
- Strong validation rules for username and password
- Proper use of translations for error messages
- Type-safe enums for user_type and gender
- Smart handling of optional fields with transform
Line range hint
106-127: LGTM! Form configuration follows best practices.Good implementation of:
- "onChange" validation mode for real-time feedback
- Sensible default values
- Proper phone number synchronization with WhatsApp
290-295: LGTM! Phone number input constraints implemented correctly.Good implementation of:
- maxLength constraint for phone numbers
- Proper disabled state for WhatsApp number when synced
Also applies to: 313-315
349-355: LGTM! Date picker implementation with proper constraints.Correctly implements:
- Future date restriction
- Reasonable minimum date (1900-01-01)
- Proper date handling with the DatePicker component
465-474: LGTM! Submit button implementation with proper states.Well implemented:
- Disabled state based on form validity and dirty state
- Loading indicator during submission
- Translated button text
|
👋 Hi, @rajku-dev, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Patient/PatientRegistration.tsx (1)
574-583: LGTM! Age validation logic looks good.The age validation logic effectively prevents negative ages and correctly calculates the year of birth. Consider adding an upper limit to prevent unrealistic ages.
onChange={(e) => { - const age = Math.max(0, Number(e.target.value)); + const age = Math.min(150, Math.max(0, Number(e.target.value))); setForm((f) => ({ ...f, age: String(age), year_of_birth: age ? new Date().getFullYear() - age : undefined, })); }}public/locale/en.json (1)
1748-1748: Consider enhancing the required field message for better UX.While "This field is required" is functional, consider making it more user-friendly and specific, e.g., "Please fill in this field" or "This information is needed".
- "this_field_is_required": "This field is required", + "this_field_is_required": "Please fill in this field",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(4 hunks)src/components/Patient/PatientRegistration.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: cypress-run (1)
src/components/Patient/PatientRegistration.tsx
[failure] 567-567:
Cannot find name 'InputWithError'.
[failure] 586-586:
Cannot find name 'InputWithError'.
🪛 GitHub Actions: Cypress Tests
src/components/Patient/PatientRegistration.tsx
[error] 567-567: Cannot find name 'InputWithError'. The component or type 'InputWithError' is not defined or imported.
🪛 GitHub Actions: Deploy Care Fe
src/components/Patient/PatientRegistration.tsx
[error] 567-567: Cannot find name 'InputWithError'
[error] 586-586: Cannot find name 'InputWithError'
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
public/locale/en.json (7)
672-672: LGTM! Clear validation message for date of birth.The message "Date of birth cannot be in future" is clear, concise, and effectively communicates the validation rule to users.
1420-1420: LGTM! Clear phone number format specification.The message "Phone number must start with +91 followed by 10 digits" clearly specifies the required format and aligns with the 13-digit limit requirement.
1868-1868: LGTM! Clear username validation rule.The message clearly explains that consecutive special characters are not allowed in usernames.
1869-1869: LGTM! Clear specification of allowed characters.The message clearly specifies which characters are allowed in usernames (lowercase letters, numbers, and specific symbols).
1870-1871: LGTM! Clear username length restrictions.The messages clearly specify both minimum (4 characters) and maximum (16 characters) length requirements for usernames.
1873-1873: LGTM! Clear rule for username boundaries.The message clearly specifies that usernames must start and end with either a letter or number.
Line range hint
672-1873: Overall excellent validation message implementation.The validation messages are well-structured, clear, and comprehensive. They effectively support all the validation requirements mentioned in the PR objectives:
- Date of birth future date restriction
- Phone number 13-digit format
- Required field validation
- Username validation rules
The messages are user-friendly and will help users understand and correct validation errors quickly.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
src/components/Patient/PatientInfoCard.tsx (1)
265-271: Consider i18n and UX improvements for the edit button.The button implementation looks good overall, with proper routing and conditional rendering. Consider these enhancements:
- Use i18n translation for "Update Encounter" text for consistency with other translated strings in the component
- Add a tooltip to explain why the button is disabled when the encounter is completed
<Button asChild variant="primary"> <Link href={`/facility/${encounter.facility.id}/patient/${patient.id}/encounter/${encounter.id}/edit_encounter`} + title={completedEncounterStatus.includes(encounter.status) ? t('encounter_edit_disabled_tooltip') : ''} > - Update Encounter + {t('encounter_edit_button')} </Link> </Button>src/Routers/routes/ConsultationRoutes.tsx (2)
45-52: Consider maintaining consistency in parameter naming.The URL parameter
:idis passed asconsultationIdprop. For better maintainability and clarity, consider keeping the naming consistent:"/facility/:facilityId/patient/:patientId/consultation/:id/consent-records": ({ facilityId, patientId, id }) => ( <PatientConsentRecords facilityId={facilityId} patientId={patientId} - consultationId={id} + id={id} /> ),
53-64: Improve route path consistency and structure.A few suggestions for improvement:
- The URL parameter uses
encounterIdwhile others use kebab-case- The trailing slash is unnecessary
- The hardcoded "encounter" type could be made dynamic if needed
- "/facility/:facilityId/patient/:patientId/encounterId/:id/files/": ({ + "/facility/:facilityId/patient/:patientId/encounter/:id/files": ({ facilityId, patientId, id, }) => ( <FileUploadPage facilityId={facilityId} patientId={patientId} encounterId={id} - type="encounter" + type={type} // Consider making this dynamic if needed /> ),src/components/ui/errors.tsx (1)
1-11: Consider accessibility and key improvements.The component looks good but could benefit from these enhancements:
- Use a more semantic HTML element like
ul/lifor error lists- Add appropriate ARIA attributes for screen readers
- Use unique error message content as key instead of array index
export function InputErrors({ errors }: { errors?: string[] }) { return errors && !!errors.length ? ( - <div className="mt-1" data-input-error> + <ul className="mt-1" role="alert" data-input-error> {errors?.map((error, i) => ( - <div key={i} className="text-red-500 text-xs"> + <li key={error} className="text-red-500 text-xs"> {error} - </div> + </li> ))} - </div> + </ul> ) : null; }src/components/ui/input-with-error.tsx (1)
4-24: Enhance accessibility with ARIA attributes and htmlFor.The component looks well-structured, but could be improved for accessibility:
export default function InputWithError(props: { label?: string; required?: boolean; errors?: string[]; children: React.ReactNode; + id?: string; }) { - const { label, errors, children, required } = props; + const { label, errors, children, required, id } = props; return ( <> {label && ( - <Label className="mb-2"> + <Label className="mb-2" htmlFor={id}> {label} - {required && <span className="text-red-500">*</span>} + {required && <span className="text-red-500" aria-label="required">*</span>} </Label> )} - {children} + <div aria-required={required} aria-invalid={!!errors?.length}> + {children} + </div> <InputErrors errors={errors} /> </> ); }src/components/Patient/allergy/list.tsx (1)
Line range hint
103-143: Consider extracting badge styling logic to separate utility functions.The badge styling logic could be simplified by moving it to separate utility functions or using a mapping object:
const STATUS_BADGE_STYLES = { active: "bg-green-100 text-green-800 border-green-200", inactive: "bg-gray-100 text-gray-800 border-gray-200", default: "bg-gray-100 text-gray-800 border-gray-200", } as const; const getStatusBadgeStyle = (status: string = "") => STATUS_BADGE_STYLES[status.toLowerCase() as keyof typeof STATUS_BADGE_STYLES] ?? STATUS_BADGE_STYLES.default;🧰 Tools
🪛 Biome (1.9.4)
[error] 103-103: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
src/pages/Organization/components/OrganizationSelector.tsx (2)
Line range hint
25-25: Address TODO comment about component renaming.The TODO comment suggests renaming to
GovtOrganizationSelector. This would better reflect the component's specific purpose.Would you like me to help create a GitHub issue to track this renaming task?
142-148: Simplify complex label translation logic.The nested ternary operation in the label prop could be simplified for better readability:
- label={t( - lastLevel - ? `SYSTEM__govt_org_type__${lastLevel.metadata?.govt_org_children_type || "default"}` - : "SYSTEM__govt_org_type__default", - )} + label={t(`SYSTEM__govt_org_type__${ + lastLevel?.metadata?.govt_org_children_type ?? "default" + }`)}src/components/Patient/PatientRegistration.tsx (2)
334-341: Consider using a phone number validation library.While the current implementation limits phone numbers to 13 digits, consider using a library like
libphonenumber-jsfor:
- Proper phone number validation
- Consistent formatting
- Country code handling
+import { parsePhoneNumber as parsePhoneNumberLib } from 'libphonenumber-js'; -if (e.target.value.length > 13) return; +const phoneNumber = parsePhoneNumberLib(e.target.value); +if (!phoneNumber?.isValid()) return;Also applies to: 375-380
450-501: Enhance date validation for better user experience.The current date input implementation could be improved:
- Add validation for valid date combinations (e.g., February 30th)
- Consider using a date picker component for better UX
- Add validation for future dates
Consider using a date picker component:
<DatePicker value={form.date_of_birth} onChange={(date) => setForm((f) => ({ ...f, date_of_birth: date }))} maxDate={new Date()} minDate={new Date('1900-01-01')} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
crowdin.yml(1 hunks)public/locale/en.json(4 hunks)src/Routers/routes/ConsultationRoutes.tsx(2 hunks)src/components/Patient/PatientInfoCard.tsx(2 hunks)src/components/Patient/PatientRegistration.tsx(8 hunks)src/components/Patient/allergy/list.tsx(1 hunks)src/components/ui/errors.tsx(1 hunks)src/components/ui/input-with-error.tsx(1 hunks)src/pages/FacilityOrganization/components/FacilityOrganizationSelector.tsx(3 hunks)src/pages/Organization/components/OrganizationSelector.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Patient/allergy/list.tsx
[error] 103-103: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (9)
src/components/Patient/PatientInfoCard.tsx (1)
26-26: LGTM!The Button import is correctly placed and properly supports the new edit encounter functionality.
src/Routers/routes/ConsultationRoutes.tsx (1)
83-86: 🛠️ Refactor suggestionAddress route inconsistencies and unused parameters.
Several issues to address:
- The route uses underscore (
questionnaire_response) instead of kebab-case- URL parameters
facilityIdandencounterIdare captured but not used- Inconsistent parameter naming (
id→responseId)- "/facility/:facilityId/patient/:patientId/encounter/:encounterId/questionnaire_response/:id": + "/facility/:facilityId/patient/:patientId/encounter/:encounterId/questionnaire-response/:id": - ({ patientId, id }) => ( + ({ facilityId, patientId, encounterId, id }) => ( <QuestionnaireResponseView - responseId={id} + id={id} patientId={patientId} + facilityId={facilityId} + encounterId={encounterId} /> ),Let's verify if these parameters are required by the component:
src/pages/Organization/components/OrganizationSelector.tsx (1)
114-134:⚠️ Potential issueAdd missing key prop to parent div in map.
The parent div in the map iteration is missing a key prop:
- {selectedLevels.map((level, index) => ( - <div> + {selectedLevels.map((level, index) => ( + <div key={level.id}>Likely invalid or redundant comment.
src/pages/FacilityOrganization/components/FacilityOrganizationSelector.tsx (1)
Line range hint
109-188: Well-structured implementation of InputWithError component!The refactoring improves error handling and maintains UI consistency across the application.
crowdin.yml (1)
2-3: Improved translation file organization!The dynamic path structure using
{{lang}}and%original_file_name%enhances maintainability of localization files.public/locale/en.json (1)
670-670: Well-structured validation messages!The added messages provide clear feedback for:
- Date of birth validation
- Phone number format requirements
- Required field indicators
- Username format requirements
This enhances the user experience by providing clear validation feedback.
Also applies to: 1415-1415, 1742-1742, 1862-1867
src/components/Patient/PatientRegistration.tsx (3)
11-11: Verify the import paths for InputWithError and InputErrors components.There was a previous build failure related to the InputWithError import. Ensure that the import paths are correct and the components are properly exported from their respective modules.
#!/bin/bash # Verify the existence and exports of the UI components ast-grep --pattern 'export $_ InputWithError' ast-grep --pattern 'export $_ InputErrors'Also applies to: 52-52
523-531: Well implemented age validation!The age validation implementation is robust:
- Prevents negative values
- Automatically calculates year of birth
- Provides immediate feedback to users
Line range hint
319-635: Well-structured form implementation with robust validation.The form implementation shows good practices:
- Consistent error handling using InputWithError
- Real-time validation feedback
- Proper state management for form fields
- Good UX with field dependencies (same phone/address checkboxes)
| @@ -86,22 +85,22 @@ export function AllergyList({ patientId, encounterId }: AllergyListProps) { | |||
| return ( | |||
| <Card className="p-0"> | |||
| <CardHeader className="px-4 py-0 pt-4"> | |||
| <CardTitle>{t("allergies")}</CardTitle> | |||
| <CardTitle>Allergies</CardTitle> | |||
There was a problem hiding this comment.
Restore i18n translations for better internationalization.
The removal of translations for table headers and title reduces the application's internationalization capabilities. Consider restoring the translations:
- <CardTitle>Allergies</CardTitle>
+ <CardTitle>{t("allergies")}</CardTitle>
// ...
- <TableHead>Allergen</TableHead>
- <TableHead>Category</TableHead>
- <TableHead>Status</TableHead>
- <TableHead>Criticality</TableHead>
- <TableHead>Created By</TableHead>
+ <TableHead>{t("allergen")}</TableHead>
+ <TableHead>{t("category")}</TableHead>
+ <TableHead>{t("status")}</TableHead>
+ <TableHead>{t("criticality")}</TableHead>
+ <TableHead>{t("created_by")}</TableHead>Also applies to: 94-98
| </TableRow> | ||
| </TableHeader> | ||
| <TableBody> | ||
| {allergies.results.map((allergy: AllergyIntolerance) => ( | ||
| <TableRow key={allergy.id}> | ||
| <TableRow> |
There was a problem hiding this comment.
Add missing key prop to mapped TableRow.
React requires a unique key prop when mapping over elements:
- <TableRow>
+ <TableRow key={allergy.id}>Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 103-103: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
|
Hey @rithviknishad , I am going to make one more PR with the changes we made, things have gone a little messy here. apologies. |
github-actions
bot
added
merge conflict
|
👋 Hi, @rajku-dev, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
Proposed Changes
CreateUserFormto track errors as user types for individual fields to improve UXCreateUserFormuntil the form values are valid according to Zod Schema definedDateFormFieldto prevent users from selecting future DOBisDirtyproperty in all react-hook-forms to prevent Unnecessary network requests@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
isDirtystate to prevent unnecessary form submissions across multiple components.DatePickerfor date of birth.UserCreateRequesttype for user creation.InputWithErrorandInputErrors.Bug Fixes