Improvements in CreateUserForm #9887
Conversation
WalkthroughThis pull request introduces comprehensive improvements to user form validation and localization. The changes span multiple files, focusing on enhancing the user registration process by adding detailed validation rules for dates of birth and phone numbers. The modifications include updating the localization JSON file with new validation messages, refactoring the user creation form to utilize the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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. |
|
@Jacobjeevan please review the changes , last pr was closed due to so many merge conflicts |
![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 (7)
src/types/user/user.ts (1)
18-33: Consider using string type for dates in API requests.While using
Datetype fordate_of_birthworks well in the frontend, it might be better to usestringtype since API requests typically serialize dates to strings. This would make the type definition more aligned with the actual API contract.- date_of_birth: Date; + date_of_birth: string;src/components/ui/date-picker.tsx (1)
19-19: Add JSDoc comments for better documentation.The
disabledprop is a powerful feature but could benefit from documentation explaining its usage and providing examples.interface DatePickerProps { date?: Date; onChange?: (date?: Date) => void; + /** + * Function to determine if a date should be disabled + * @param date The date to check + * @returns true if the date should be disabled, false otherwise + * @example + * // Disable future dates + * disabled={(date) => date > new Date()} + */ disabled?: (date: Date) => boolean; }Also applies to: 22-22, 48-48
src/components/Users/CreateUserForm.tsx (3)
175-188: Consider adding retry logic for API calls.The mutation setup looks good, but consider adding retry logic for transient failures:
const { mutate: createUser, isPending } = useMutation({ mutationFn: mutate(UserApi.create), + retry: (failureCount, error) => { + return failureCount < 3 && error.status >= 500; + }, onSuccess: (user: UserBase) => {
342-347: Add input pattern for phone number validation.Consider adding the
patternattribute to reinforce the phone number format on the client side.<Input type="tel" placeholder="+91XXXXXXXXXX" maxLength={13} + pattern="^\+91[0-9]{10}$" {...field} />
402-408: Consider extracting date constraints to constants.The date validation logic is duplicated between the Zod schema and the DatePicker component. Consider extracting these constraints to constants.
+const DATE_CONSTRAINTS = { + MIN_DATE: new Date("1900-01-01"), + MAX_DATE: new Date(), +} as const; + <DatePicker date={field.value} onChange={(date) => field.onChange(date)} disabled={(date) => - date > new Date() || date < new Date("1900-01-01") + date > DATE_CONSTRAINTS.MAX_DATE || date < DATE_CONSTRAINTS.MIN_DATE } />public/locale/en.json (2)
1495-1495: Consider enhancing the phone number validation message.While the message is clear, it could be more user-friendly by providing an example format.
- "phone_number_must_start": "Phone number must start with +91 followed by 10 digits", + "phone_number_must_start": "Phone number must start with +91 followed by 10 digits (e.g., +911234567890)",
1999-2004: Consider consolidating username validation messages.The current implementation has separate messages for each username validation rule. Consider combining them into a single, comprehensive message to improve user experience.
- "username_consecutive_special_characters": "Username can't contain consecutive special characters", - "username_contain_lowercase_special": "Username can only contain lowercase letters, numbers, and . _ -", - "username_less_than": "Username must be less than 16 characters", - "username_more_than": "Username must be at least 4 characters", - "username_start_end_letter_number": "Username must start and end with a letter or number", + "username_requirements": "Username must: be 4-16 characters long, contain only lowercase letters, numbers, and . _ -, start and end with a letter or number, and not have consecutive special characters",This consolidation would:
- Reduce cognitive load by presenting all rules at once
- Make it easier for users to create a valid username on their first try
- Reduce the number of validation cycles needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
public/locale/en.json(4 hunks)src/components/Users/CreateUserForm.tsx(14 hunks)src/components/ui/date-picker.tsx(2 hunks)src/types/user/user.ts(1 hunks)src/types/user/userApi.ts(2 hunks)
🔇 Additional comments (5)
src/types/user/userApi.ts (1)
3-3: LGTM! Type safety improvement.The change from
UserBasetoUserCreateRequestfor the create method's request body type improves type safety by using a more specific type for user creation.Also applies to: 15-15
src/components/Users/CreateUserForm.tsx (2)
51-111: Strong validation schema with i18n support.The Zod schema implementation is thorough and well-structured, with proper internationalization of error messages.
517-525: LGTM! Good UX with loading state.The submit button implementation is well done with:
- Proper disable conditions
- Loading indicator
- Internationalized text
public/locale/en.json (2)
699-699: LGTM! Clear and concise validation message.The validation message for date of birth is straightforward and effectively communicates the constraint to users.
1875-1875: LGTM! Standard required field message.The message follows the common pattern for required field validation.
|
👋 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 (4)
src/components/Users/CreateUserForm.tsx (4)
76-77: Consider making phone number validation more flexible.The phone number validation is currently hardcoded to accept only Indian numbers (+91). Consider making this more flexible to support international phone numbers or configurable based on the deployment region.
- .regex(/^\+91[0-9]{10}$/, t("phone_number_must_start")), + .regex(/^\+[1-9]\d{1,14}$/, t("invalid_phone_number")),Also applies to: 80-84
186-199: Enhance error handling in mutation.The error handling could be more robust:
- Consider adding a generic error message for unexpected error types
- Add type safety for error.cause
onError: (error) => { - const errors = (error.cause?.errors as any[]) || []; + if (!error.cause?.errors || !Array.isArray(error.cause.errors)) { + toast.error(t("unexpected_error")); + return; + } + const errors = error.cause.errors as Array<{ + loc: string[]; + ctx: { error: string }; + }>; errors.forEach((err) => { const field = err.loc[0]; form.setError(field, { message: err.ctx.error }); }); },
408-414: Enhance DatePicker accessibility.The DatePicker component should include aria-label and proper date format announcement.
<DatePicker date={field.value} onChange={(date) => field.onChange(date)} + aria-label={t("date_of_birth")} disabled={(date) => date > new Date() || date < new Date("1900-01-01") } />
523-531: Improve submit button feedback.The button could provide more specific feedback about what's preventing submission.
<Button type="submit" className="w-full" disabled={ !form.formState.isDirty || !form.formState.isValid || isPending } + title={ + !form.formState.isDirty + ? t("no_changes_made") + : !form.formState.isValid + ? t("please_fix_form_errors") + : "" + } > {isPending && <Loader2 className="mr-2 h-4 w-4 animate-spin" />} - {t("create")} + {t(isPending ? "creating" : "create")} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(3 hunks)src/components/Users/CreateUserForm.tsx(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: CodeQL-Build
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
src/components/Users/CreateUserForm.tsx (2)
Line range hint
1-534: Overall implementation looks solid! 👍The form implementation is well-structured with proper validation, error handling, and user feedback. The use of react-query for mutations and zod for validation provides a robust foundation.
87-93: Review date validation range.The date validation allows dates from 1900, which might need adjustment based on business requirements. Also, consider adding a minimum age requirement.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/Users/CreateUserForm.tsx (1)
Line range hint
117-125: Adjust phone number default value to match validation pattern.The default value
"+91"for phone numbers doesn't match the validation pattern/^\+91[0-9]{10}$/. This could cause initial validation errors. Consider either:
- Removing the default value, or
- Setting it to an empty string
🧹 Nitpick comments (1)
src/components/Users/CreateUserForm.tsx (1)
192-198: Improve type safety in error handling.The error handling code makes assumptions about the error structure without proper type checking. Consider:
- Adding proper type definitions for the error response
- Adding null checks before accessing error properties
- const errors = (error.cause?.errors as any[]) || []; + type ApiError = { loc: string[]; ctx: { error: string } }; + const errors = ((error.cause?.errors as ApiError[]) || []) + .filter((err): err is ApiError => + Array.isArray(err?.loc) && typeof err?.ctx?.error === 'string' + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Users/CreateUserForm.tsx(16 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 (2)
src/components/Users/CreateUserForm.tsx (2)
94-94: Use GENDER_TYPES constant for schema validation.The gender validation is hardcoded in the schema while the UI uses the GENDER_TYPES constant, which could lead to inconsistencies if the gender types are updated.
523-531: LGTM! Well-implemented submit button.The submit button implementation includes:
- Proper loading state with spinner
- Correct disable conditions based on form state
- Clear visual feedback
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/Users/CreateUserForm.tsx (2)
86-92:⚠️ Potential issueFix date of birth validation issues.
The current date validation has several problems:
- Using string type instead of Date
- Incorrect date comparison
- Missing minimum date validation
As noted in previous feedback, also consider improving the date selection UX. Apply this fix:
date_of_birth: z - .string({ + .date({ required_error: t("this_field_is_required"), }) + .min(new Date("1900-01-01"), t("date_too_far_in_past")) - .refine((dob) => dob <= new Date().toISOString(), { + .max(new Date(), { message: t("date_of_birth_cannot_be_in_future"), }),
93-93:⚠️ Potential issueUse GENDER_TYPES constant for schema validation.
The gender enum is hardcoded in the schema while GENDER_TYPES constant is used in the UI. This could lead to inconsistencies.
- gender: z.enum(["male", "female", "transgender", "non_binary"]), + gender: z.enum(GENDER_TYPES.map(g => g.id)),
🧹 Nitpick comments (1)
src/components/Users/CreateUserForm.tsx (1)
74-84: Make phone number validation more flexible.The current validation is hardcoded for Indian numbers (+91). Consider making it more flexible for international usage.
+ const PHONE_REGEX = { + IN: /^\+91[0-9]{10}$/, + // Add more country patterns + }; phone_number: z .string() - .regex(/^\+91[0-9]{10}$/, t("phone_number_must_start")), + .regex(PHONE_REGEX.IN, t("invalid_phone_number_format")), alt_phone_number: z .string() .refine( - (val) => val === "" || /^\+91[0-9]{10}$/.test(val), + (val) => val === "" || PHONE_REGEX.IN.test(val), - t("phone_number_must_start"), + t("invalid_phone_number_format"), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/CreateUserForm.tsx(15 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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: CodeQL-Build
- GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
src/components/Users/CreateUserForm.tsx (2)
200-217: Well implemented form submission!Great job on:
- Type-safe form submission using
z.infer- Proper loading state with spinner
- Button disabled state based on form validity
Also applies to: 516-524
347-352: Well structured phone input implementation!Good practices implemented:
- Correct input type="tel"
- Appropriate maxLength matching the format
- Clear placeholder showing expected format
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Users/CreateUserForm.tsx (3)
75-76: Consider making phone number prefix configurable.The phone number validation is hardcoded for Indian numbers (+91). Consider making this configurable to support international phone numbers.
+ const PHONE_NUMBER_REGEX = process.env.REACT_APP_PHONE_REGEX || /^\+91[0-9]{10}$/; phone_number: z .string() - .regex(/^\+91[0-9]{10}$/, t("phone_number_must_start")), + .regex(PHONE_NUMBER_REGEX, t("phone_number_must_start")),
141-147: Move password match validation to schema.The password match validation in useEffect should be moved to the schema for consistency and to avoid maintaining validation logic in multiple places.
The schema already has this validation. Remove the duplicate validation from useEffect:
- if (password?.length && c_password?.length && password !== c_password) { - form.setError("c_password", { message: t("password_mismatch") }); - form.setError("password", { message: t("password_mismatch") }); - } else { - form.clearErrors("c_password"); - form.clearErrors("password"); - }
356-361: Enhance phone number input UX.Consider using a phone number input component with formatting and validation:
- Use a phone input library like
react-phone-number-input- Add proper formatting as user types
- Show country selection dropdown
Example implementation:
- <Input - type="tel" - placeholder="+91XXXXXXXXXX" - maxLength={13} - {...field} - /> + <PhoneInput + international + defaultCountry="IN" + placeholder="Enter phone number" + value={field.value} + onChange={field.onChange} + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/CreateUserForm.tsx(17 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (3)
src/components/Users/CreateUserForm.tsx (3)
93-93: Use GENDER_TYPES constant for schema validation.The gender enum is hardcoded in the schema while GENDER_TYPES constant is used in the UI. This could lead to inconsistencies.
- gender: z.enum(["male", "female", "transgender", "non_binary"]), + gender: z.enum(GENDER_TYPES.map(g => g.id)),
200-206: Improve error handling in mutation setup.The error handling could be more robust:
- The type assertion
as any[]in error handling is unsafe- No fallback error message when
errorsarray is emptyonError: (error) => { - const errors = (error.cause?.errors as any[]) || []; + const errors = Array.isArray(error.cause?.errors) ? error.cause.errors : []; errors.forEach((err) => { const field = err.loc[0]; form.setError(field, { message: err.ctx.error }); }); + if (errors.length === 0) { + toast.error(t("something_went_wrong")); + } },
525-533: LGTM! Good UX improvements.The submit button implementation looks good:
- Proper disable conditions based on form state
- Loading indicator during submission
- Translated button text
it was due to sol.mp4 |
|
let's not move it to useEffect. let's stick with refine itself. however, refine would get triggered when form is attempted to submit right? so what's the issue? why was the button disabled in the first place which prevented submitting it? |
rithviknishad
added
changes required
and removed
invalid
issue.mp4.refine is not triggering on changing the password fields, so i had to apply the hack |
@rajku-dev, I think you missed this line from rithvik's comment |
The problem is, password has mismatch error that makes the submit button disable(since we are doing invalid check), but not showing under input field |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/CreateUserForm.tsx(17 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Users/CreateUserForm.tsx (3)
Learnt from: rajku-dev
PR: ohcnetwork/care_fe#9887
File: src/components/Users/CreateUserForm.tsx:153-159
Timestamp: 2025-01-14T09:56:07.985Z
Learning: In react-hook-form, form.formState.isValid being false with an empty form.formState.errors object typically indicates that either not all required fields are filled, some fields haven't been "touched", or there's pending async validation. The errors object only contains validation errors for fields that have been validated.
Learnt from: rajku-dev
PR: ohcnetwork/care_fe#9887
File: src/components/Users/CreateUserForm.tsx:93-93
Timestamp: 2025-01-14T09:22:13.878Z
Learning: The suggestion to use GENDER_TYPES.map(g => g.id) for gender enum validation in CreateUserForm.tsx was incorrect and caused an error.
Learnt from: rajku-dev
PR: ohcnetwork/care_fe#9887
File: src/components/Users/CreateUserForm.tsx:93-93
Timestamp: 2025-01-14T09:22:13.878Z
Learning: In CreateUserForm.tsx, the gender validation schema uses string literals that match GENDER_TYPES.map(g => g.id). Using GENDER_TYPES directly with z.enum() fails because it's marked with 'as const' which makes it a readonly tuple type incompatible with Zod's enum.
🪛 eslint
src/components/Users/CreateUserForm.tsx
[error] 581-581: Replace <Loader2·className="mr-2·h-4·w-4·animate-spin"·data-cy="submit-user-form"·/> with (⏎············<Loader2⏎··············className="mr-2·h-4·w-4·animate-spin"⏎··············data-cy="submit-user-form"⏎············/>⏎··········)
(prettier/prettier)
🪛 GitHub Check: lint
src/components/Users/CreateUserForm.tsx
[failure] 581-581:
Replace <Loader2·className="mr-2·h-4·w-4·animate-spin"·data-cy="submit-user-form"·/> with (⏎············<Loader2⏎··············className="mr-2·h-4·w-4·animate-spin"⏎··············data-cy="submit-user-form"⏎············/>⏎··········)
🪛 GitHub Actions: Lint Code Base
src/components/Users/CreateUserForm.tsx
[error] 581-581: Formatting error: Loader2 component needs to be properly formatted across multiple lines according to prettier rules
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/components/Users/CreateUserForm.tsx (3)
221-238: Verify API contract for user creation.The submission logic looks good, but let's verify that all required fields are being sent correctly to the backend.
Run this script to check the API contract:
✅ Verification successful
API contract verification successful
The form submission matches the UserCreateRequest type definition exactly, with all required fields being sent and optional fields properly handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the API contract for user creation # Search for UserCreateRequest type definition and API usage # Find UserCreateRequest type definition echo "=== UserCreateRequest Type Definition ===" ast-grep --pattern 'type UserCreateRequest = { $$$ }' # Find API usage patterns echo -e "\n=== API Usage Patterns ===" rg -A 5 'UserApi.create'Length of output: 1523
153-159: 🛠️ Refactor suggestionRemove duplicate password validation from useEffect.
The password validation logic is duplicated between the schema and useEffect hook. The schema already handles this validation with
.refine(), making this useEffect unnecessary and potentially causing race conditions.Remove the password validation from useEffect as it's already handled by the schema:
- if (password?.length && c_password?.length && password !== c_password) { - form.setError("c_password", { message: t("password_mismatch") }); - form.setError("password", { message: t("password_mismatch") }); - } else { - form.clearErrors("c_password"); - form.clearErrors("password"); - }Likely invalid or redundant comment.
206-219: 🛠️ Refactor suggestionImprove mutation error handling robustness.
The current error handling has two issues:
- Unsafe type assertion with
as any[]- No fallback error message when the errors array is empty
Apply these improvements:
onError: (error) => { - const errors = (error.cause?.errors as any[]) || []; + const errors = Array.isArray(error.cause?.errors) ? error.cause.errors : []; errors.forEach((err) => { const field = err.loc[0]; - form.setError(field, { message: err.ctx?.error || err.msg }); + form.setError(field, { message: err.ctx?.error || err.msg || t("unknown_error") }); }); + if (errors.length === 0) { + toast.error(t("something_went_wrong")); + } },Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Users/CreateUserForm.tsx (2)
86-100: Enhance date validation error message.The error message doesn't accurately reflect both minimum and maximum date constraints.
.refine( (dob) => { const date = new Date(dob); + if (isNaN(date.getTime())) return false; const minDate = new Date(); minDate.setFullYear(minDate.getFullYear() - 120); return minDate <= date && date <= new Date(); }, { - message: t("date_of_birth_cannot_be_in_future"), + message: t("invalid_date_of_birth"), }, ),
206-219: Improve error handling in mutation setup.The error handling could be more robust:
- The type assertion
as any[]is unsafe- No fallback error message when
errorsarray is emptyonError: (error) => { - const errors = (error.cause?.errors as any[]) || []; + const errors = Array.isArray(error.cause?.errors) ? error.cause.errors : []; errors.forEach((err) => { const field = err.loc[0]; - form.setError(field, { message: err.ctx?.error || err.msg }); + form.setError(field, { message: err.ctx?.error || err.msg || t("unknown_error") }); }); + if (errors.length === 0) { + toast.error(t("something_went_wrong")); + } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/CreateUserForm.tsx(17 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Users/CreateUserForm.tsx (3)
Learnt from: rajku-dev
PR: ohcnetwork/care_fe#9887
File: src/components/Users/CreateUserForm.tsx:153-159
Timestamp: 2025-01-14T09:56:07.985Z
Learning: In react-hook-form, form.formState.isValid being false with an empty form.formState.errors object typically indicates that either not all required fields are filled, some fields haven't been "touched", or there's pending async validation. The errors object only contains validation errors for fields that have been validated.
Learnt from: rajku-dev
PR: ohcnetwork/care_fe#9887
File: src/components/Users/CreateUserForm.tsx:93-93
Timestamp: 2025-01-14T09:22:13.878Z
Learning: The suggestion to use GENDER_TYPES.map(g => g.id) for gender enum validation in CreateUserForm.tsx was incorrect and caused an error.
Learnt from: rajku-dev
PR: ohcnetwork/care_fe#9887
File: src/components/Users/CreateUserForm.tsx:93-93
Timestamp: 2025-01-14T09:22:13.878Z
Learning: In CreateUserForm.tsx, the gender validation schema uses string literals that match GENDER_TYPES.map(g => g.id). Using GENDER_TYPES directly with z.enum() fails because it's marked with 'as const' which makes it a readonly tuple type incompatible with Zod's enum.
🔇 Additional comments (5)
src/components/Users/CreateUserForm.tsx (5)
3-4: LGTM! Good choice of dependencies.The addition of
useMutationandLoader2components improves API handling and user feedback.
73-76: LGTM! Improved validation messages.The email and phone number validation messages are now more descriptive and user-friendly.
Line range hint
128-144: LGTM! Improved form configuration.The "onChange" mode provides better real-time feedback, and default values improve UX.
221-238: LGTM! Clean and type-safe implementation.The submission handler correctly maps form data to the API request structure.
574-583: 🛠️ Refactor suggestionFix button formatting according to prettier rules.
The Loader2 component needs to be properly formatted across multiple lines.
<Button type="submit" className="w-full" disabled={ !form.formState.isDirty || !form.formState.isValid || isPending } > - {isPending && <Loader2 className="mr-2 h-4 w-4 animate-spin" />}{" "} + {isPending && ( + <Loader2 + className="mr-2 h-4 w-4 animate-spin" + data-cy="submit-user-form" + /> + )}{" "} {t("create")} </Button>Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Users/CreateUserForm.tsx (2)
76-83: Improve phone number validation robustness.The current validation:
- Only supports Indian numbers (+91)
- Doesn't validate the actual phone number format beyond length
Consider using a more comprehensive validation:
phone_number: z .string() - .regex(/^\+91[0-9]{10}$/, t("phone_number_must_start")), + .regex(/^\+91[1-9][0-9]{9}$/, t("phone_number_must_start")),
101-105: Improve type safety of gender validation.The current implementation uses a type assertion. Consider using a const assertion instead:
- gender: z.enum( - GENDER_TYPES.map((gender) => gender.id) as [ - (typeof GENDER_TYPES)[number]["id"], - ], - ), + gender: z.enum( + GENDER_TYPES.map((gender) => gender.id) as const, + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/CreateUserForm.tsx(14 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Users/CreateUserForm.tsx (3)
Learnt from: rajku-dev
PR: ohcnetwork/care_fe#9887
File: src/components/Users/CreateUserForm.tsx:153-159
Timestamp: 2025-01-14T09:56:07.985Z
Learning: In react-hook-form, form.formState.isValid being false with an empty form.formState.errors object typically indicates that either not all required fields are filled, some fields haven't been "touched", or there's pending async validation. The errors object only contains validation errors for fields that have been validated.
Learnt from: rajku-dev
PR: ohcnetwork/care_fe#9887
File: src/components/Users/CreateUserForm.tsx:93-93
Timestamp: 2025-01-14T09:22:13.878Z
Learning: The suggestion to use GENDER_TYPES.map(g => g.id) for gender enum validation in CreateUserForm.tsx was incorrect and caused an error.
Learnt from: rajku-dev
PR: ohcnetwork/care_fe#9887
File: src/components/Users/CreateUserForm.tsx:93-93
Timestamp: 2025-01-14T09:22:13.878Z
Learning: In CreateUserForm.tsx, the gender validation schema uses string literals that match GENDER_TYPES.map(g => g.id). Using GENDER_TYPES directly with z.enum() fails because it's marked with 'as const' which makes it a readonly tuple type incompatible with Zod's enum.
🔇 Additional comments (4)
src/components/Users/CreateUserForm.tsx (4)
3-4: LGTM!The new imports support the mutation-based API calls and loading indicator.
210-227: LGTM!The form submission logic is well-structured and type-safe.
563-570:⚠️ Potential issueFix Loader2 component formatting.
The component needs to be properly formatted according to prettier rules.
<Button type="submit" className="w-full" data-cy="submit-user-form" disabled={!form.formState.isDirty || isPending} > - {isPending && <Loader2 className="mr-2 h-4 w-4 animate-spin" />}{" "} + {isPending && ( + <Loader2 + className="mr-2 h-4 w-4 animate-spin" + data-cy="submit-user-form" + /> + )}{" "} {t("create_user")} </Button>Likely invalid or redundant comment.
195-208: 🛠️ Refactor suggestionImprove error handling robustness.
The current error handling has two issues:
- Unsafe type assertion with
as any[]- No fallback error message when
errorsarray is emptyonError: (error) => { - const errors = (error.cause?.errors as any[]) || []; + const errors = Array.isArray(error.cause?.errors) ? error.cause.errors : []; errors.forEach((err) => { const field = err.loc[0]; - form.setError(field, { message: err.ctx?.error || err.msg }); + form.setError(field, { message: err.ctx?.error || err.msg || t("unknown_error") }); }); + if (errors.length === 0) { + toast.error(t("something_went_wrong")); + } },Likely invalid or redundant comment.
|
removed the |
rithviknishad
added
needs testing
needs review
and removed
question
|
@rajku-dev use proper PR heading |
rajku-dev
changed the title
|
LGTM @rithviknishad can you review it |
Proposed Changes
CreateUserFormand add dirtyState to all react-hook-form #9749@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Localization
User Experience