Fix: Switch to shadcn form in userResetPassword #9708
Conversation
WalkthroughThis pull request involves a significant refactoring of form management in the application. The primary changes include removing the custom Changes
Assessment against linked 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 (3)
src/components/Users/UserResetPassword.tsx (3)
28-50: Use i18n for user-facing error messages
The validation messages within thePasswordSchema(e.g., "Please enter current password", "New password must be at least 8 characters long", etc.) are hardcoded. To maintain consistency with the rest of the application and support localization, consider usingt()or otherwise integrating them into your i18n workflow.
136-160: Avoid duplicate validations
You are using bothzodto validate passwords and thevalidateRulefunction to check password requirements on focus. This might create unnecessary duplication and potential maintenance overhead. Consider consolidating all password checks in one place (preferablyzod) and displaying those error messages consistently.
95-197: Consider adding a “show/hide password” toggle
The current form effectively captures password input and provides helpful validation. However, offering a “show/hide password” toggle can enhance user experience and reduce entry errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Form/Form.tsx(0 hunks)src/components/Users/UserResetPassword.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- src/components/Form/Form.tsx
🔇 Additional comments (1)
src/components/Users/UserResetPassword.tsx (1)
200-208: Good approach to toggling edit mode
Toggling the password edit state withsetIsEditingis a clean, straightforward approach and helps keep the UI clear. No issues found here.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/components/Users/UserResetPassword.tsx (3)
29-51: Add i18n support for validation messagesThe validation messages in the schema are hardcoded in English. Consider using the translation function to support internationalization.
const PasswordSchema = z .object({ current_password: z .string() - .min(1, { message: "Please enter current password" }), + .min(1, { message: t("please_enter_current_password") }), new_password_1: z .string() - .min(8, { message: "New password must be at least 8 characters long" }) + .min(8, { message: t("password_length_validation") }) - .regex(/\d/, { message: "Password must contain at least one number" }) + .regex(/\d/, { message: t("password_number_validation") }) .regex(/[a-z]/, { - message: "Password must contain at least one lowercase letter", + message: t("password_lowercase_validation"), }) .regex(/[A-Z]/, { - message: "Password must contain at least one uppercase letter", + message: t("password_uppercase_validation"), }), new_password_2: z .string() - .min(1, { message: "Please confirm your new password" }), + .min(1, { message: t("please_confirm_password") }), })
70-90: Enhance mutation implementation and error handlingA few suggestions to improve the mutation implementation:
- Use
isPendingfromuseMutationinstead of managing loading state manually- Consider not resetting the form on error to allow users to fix mistakes
- Add type safety for the error response
- const { mutate: resetUserPasswordMutate } = useMutation({ + const { mutate: resetUserPasswordMutate, isPending } = useMutation({ mutationFn: async (formData: UpdatePasswordForm) => { const response = await mutate(routes.updatePassword, { silent: true })( formData, ); if ("errors" in response) { throw response; } return response; }, onSuccess: (data: any) => { Notification.Success({ msg: data?.message as string }); resetPasswordForm.reset(); }, onError: (error: any) => { + type ErrorResponse = { + response?: { + data?: { + message?: string; + }; + }; + }; + const typedError = error as ErrorResponse; const errorMessage = - error?.response?.data?.message || t("password_update_error"); + typedError?.response?.data?.message || t("password_update_error"); Notification.Error({ msg: errorMessage }); - resetPasswordForm.reset(); }, });
212-220: Use shadcn's button variants for consistent stylingInstead of custom styling, consider using shadcn's button variants for consistent styling across the application.
<Button onClick={() => setIsEditing(!isEditing)} type="button" id="change-edit-password-button" - className="flex items-center gap-2 rounded-sm border border-gray-100 bg-white px-3 py-1.5 text-sm text-[#009D48] shadow-sm hover:bg-gray-50" + variant="outline" + className="flex items-center gap-2" > <CareIcon icon={isEditing ? "l-times" : "l-edit"} className="h-4 w-4" /> {isEditing ? t("cancel") : t("change_password")} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/UserResetPassword.tsx(1 hunks)
🔇 Additional comments (1)
src/components/Users/UserResetPassword.tsx (1)
92-100: Unify password field naming conventionThe form uses
current_passwordwhile the API expectsold_password. This creates unnecessary mapping and potential confusion.Consider unifying the naming convention throughout the codebase:
- Update the API to use
current_password, or- Update the form to use
old_password
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Users/UserResetPassword.tsx (1)
136-161: 🛠️ Refactor suggestionRemove duplicate password validation logic.
The password requirements are already defined in the zod schema. The manual validation here duplicates that logic and could become inconsistent.
Use the form's built-in error display instead:
- {isPasswordFieldFocused && ( - <div - className="text-small mt-2 pl-2 text-secondary-500" - aria-live="polite" - > - {validateRule( - field.value.length >= 8, - t("password_length_validation"), - !field.value, - )} - {validateRule( - /[a-z]/.test(field.value), - t("password_lowercase_validation"), - !field.value, - )} - {validateRule( - /[A-Z]/.test(field.value), - t("password_uppercase_validation"), - !field.value, - )} - {validateRule( - /\d/.test(field.value), - t("password_number_validation"), - !field.value, - )} - </div> - )} + <FormMessage />
🧹 Nitpick comments (1)
src/components/Users/UserResetPassword.tsx (1)
29-51: LGTM! Well-structured password validation schema.The zod schema implementation is clean and includes all necessary validation rules. The error messages are clear and user-friendly.
Consider using t() for internationalization in error messages to maintain consistency with the rest of the application:
old_password: z .string() - .min(1, { message: "Please enter current password" }), + .min(1, { message: t("please_enter_current_password") }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(1 hunks)src/components/Users/UserResetPassword.tsx(1 hunks)
🔇 Additional comments (2)
src/components/Users/UserResetPassword.tsx (1)
189-196: LGTM! Proper button state handling.The submit button correctly uses:
isDirtyfor the disabled stateisPendingfor the loading statepublic/locale/en.json (1)
1266-1266: Verify the terminology change impact.The change from "current_password" to "old_password" should be verified across the application to ensure consistency.
Run this script to check for any remaining usages of "current_password":
✅ Verification successful
The previous search might have missed some occurrences. Let's try a broader search pattern to catch variations in naming and also look at the AST to find password-related fields. This will help us better understand the password terminology usage across the codebase.
Terminology change is consistent with implementation
The search results confirm that the change from "current_password" to "old_password" is actually consistent with the codebase implementation. The form component
UserResetPassword.tsxalready usesold_passwordin:
- Form schema validation
- Form field definitions
- API request model (
UpdatePasswordForm)- Form field rendering
The change in the locale file from "current_password" to "old_password" aligns with the existing implementation, making the terminology consistent between the code and the UI.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for remaining "current_password" occurrences echo "Searching for 'current_password' in code..." rg "current_password" -g "!*.json" echo "Searching for CurrentPassword in code..." rg "CurrentPassword" -g "!*.json"Length of output: 262
Script:
#!/bin/bash # Let's try a more comprehensive search for password-related terms echo "Searching for password-related terms in code files..." rg -i "(?:old|current)[\s_-]?password" --type-not json -A 2 -B 2 echo -e "\nSearching for password form field references..." ast-grep --pattern 'password'Length of output: 4163
the context file is used by other file like |
|
delete those usages 🙂 we don't need it. let's get rid of weird things. |
Line 42 in b36f39f |
- Introduced a new PermissionContext to manage user permissions and super admin status across the application. - Updated AppRouter to utilize PermissionProvider for managing permissions. - Changed UserModel to store permissions as strings instead of objects. - Refactored OrganizationFacilities and OrganizationPatients components to use updated permission checks and organization identifiers. - Enhanced OrganizationLayout to conditionally render navigation items based on user permissions. This commit improves the permission management system and ensures consistent handling of user permissions throughout the application.
…thub.com/AdityaJ2305/care_fe into issue/9683/userResetPassword_form_switch
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Users/UserResetPassword.tsx (1)
150-153: Simplify nested optional chaining.The error message check can be simplified using optional chaining.
- {form.formState.errors?.new_password_1?.message && - form.formState.errors?.new_password_1?.message.includes( - t("new_password_same_as_old"), - )} + {form.formState.errors?.new_password_1?.message?.includes( + t("new_password_same_as_old") + )}🧰 Tools
🪛 Biome (1.9.4)
[error] 150-153: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(3 hunks)src/components/Form/Form.tsx(1 hunks)src/components/Users/UserResetPassword.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Users/UserResetPassword.tsx
[error] 150-153: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: lint
src/components/Users/UserResetPassword.tsx
[failure] 21-21:
Insert ⏎
src/components/Form/Form.tsx
[failure] 170-170:
Delete ⏎
🪛 eslint
src/components/Users/UserResetPassword.tsx
[error] 21-21: Insert ⏎
(prettier/prettier)
src/components/Form/Form.tsx
[error] 170-171: Delete ⏎
(prettier/prettier)
🪛 GitHub Actions: Lint Code Base
src/components/Form/Form.tsx
[error] 170-170: Prettier formatting error: Invalid line break detected
🪛 GitHub Actions: Cypress Tests
src/components/Form/Form.tsx
[error] 9-9: Cannot find module '@/components/Form/FormContext' or its corresponding type declarations.
🪛 GitHub Actions: Deploy Care Fe
src/components/Form/Form.tsx
[error] 9-9: Cannot find module '@/components/Form/FormContext' or its corresponding type declarations
🔇 Additional comments (5)
src/components/Form/Form.tsx (1)
Line range hint
1-170: Verify the impact of Form component removal across the codebase.The removal of this generic Form component in favor of shadcn forms is a good architectural decision. However, we should ensure all usages are migrated.
✅ Verification successful
Let me verify the usage patterns to better understand the impact.
Let me try another search to properly check the Form-related imports and their usage.
Let me try one more time with corrected syntax:
Let me try one final search without file type specifications:
The Form component being removed appears to be already replaced
Based on the search results, I can see that the Form component usages in the codebase are already using the new shadcn Form component, as evidenced by the usage patterns like
<Form {...form}>which is typical of React Hook Form with shadcn/ui components. The imports from "@/components/Form/Form" were not found in the current codebase, suggesting the migration has already been completed.Key observations:
- All Form usages follow the pattern
<Form {...form}>which is consistent with shadcn/ui form implementation- No direct imports from '@/components/Form/Form' were found
- The Form component is being used in various features like PatientLogin, UserResetPassword, CreateFacilityForm, etc., but all appear to be using the new implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining imports or usages of the Form component echo "Searching for Form component imports..." rg -l "import.*Form.*from.*@/components/Form/Form" echo "Searching for Form component usage..." ast-grep --pattern 'Form $props={$_}'Length of output: 1521
Script:
#!/bin/bash # Check a sample of files to understand usage patterns echo "Checking sample usage patterns..." echo "=== PatientLogin.tsx ===" rg -A 5 "import.*Form.*from.*@/components/Form/Form" src/pages/Appoinments/auth/PatientLogin.tsx echo -e "\n=== ResourceCreate.tsx ===" rg -A 5 "import.*Form.*from.*@/components/Form/Form" src/components/Resource/ResourceCreate.tsx echo -e "\n=== FileUpload.tsx ===" rg -A 5 "import.*Form.*from.*@/components/Form/Form" src/components/Files/FileUpload.tsx # Check if there are any other Form-related imports that might be affected echo -e "\nChecking other Form-related imports..." rg "import.*from.*@/components/Form/" --type ts --type tsxLength of output: 1800
Script:
#!/bin/bash # Search for Form-related imports with proper file type handling echo "Checking Form-related imports..." rg "import.*from.*@/components/Form/" --type typescript --type tsx # Check for actual Form component usage echo -e "\nChecking Form component usage patterns..." rg "<Form\b" --type typescript --type tsx -B 2 -A 2 # Check for other potential Form component references echo -e "\nChecking Form component references..." rg "\bForm\b.*=.*{" --type typescript --type tsx -B 2 -A 2Length of output: 610
Script:
#!/bin/bash # Search for actual Form component usage with fixed patterns echo "Checking Form component usage..." rg -t ts -t tsx "<Form" -B 2 -A 2 # Check imports from Form directory echo -e "\nChecking Form directory imports..." rg -t ts -t tsx "from ['\"]@/components/Form/" -B 1 # Check Form component assignments echo -e "\nChecking Form component assignments..." rg -t ts -t tsx "const Form" -B 1 -A 3Length of output: 469
Script:
#!/bin/bash # Search for Form component usage in the codebase echo "Checking direct Form component usage..." rg "<Form\s" -B 2 -A 2 # Check Form-related imports and dependencies echo -e "\nChecking Form component imports and dependencies..." rg "from '@/components/Form/Form'" -B 1 # Check for Form component exports and re-exports echo -e "\nChecking Form exports..." rg "export.*Form.*from '@/components/Form/Form'"Length of output: 4095
🧰 Tools
🪛 GitHub Check: lint
[failure] 170-170:
Delete⏎🪛 GitHub Actions: Lint Code Base
[error] 170-170: Prettier formatting error: Invalid line break detected
src/components/Users/UserResetPassword.tsx (4)
205-212: LGTM! Button implementation follows best practices.The button correctly:
- Uses isPending for loading state
- Disables when form is not dirty
- Shows appropriate loading text
74-80: 🛠️ Refactor suggestionSimplify mutation implementation.
Based on the established patterns:
- Use the inferred types from the routes schema instead of
any- Let the global error handler manage error notifications
- const { mutate: resetPassword, isPending } = useMutation({ - mutationFn: mutate(routes.updatePassword), - onSuccess: () => { - toast.success(t("password_updated")); - form.reset(); - }, - }); + const { mutate: resetPassword, isPending } = useMutation({ + mutationFn: mutate(routes.updatePassword), + onSuccess: (data) => { + toast.success(data.message); + form.reset(); + } + });Likely invalid or redundant comment.
29-55: 🛠️ Refactor suggestionExtract translation calls from schema definition.
The current implementation evaluates translations only once when the schema is defined, which won't update if the language changes at runtime.
Extract the messages to constants:
+ const passwordMessages = { + current: t("please_enter_current_password"), + length: t("password_length_validation"), + number: t("password_number_validation"), + lowercase: t("password_lowercase_validation"), + uppercase: t("password_uppercase_validation"), + confirm: t("please_enter_confirm_password"), + mismatch: t("password_mismatch"), + sameAsOld: t("new_password_same_as_old"), + }; const PasswordSchema = z .object({ old_password: z .string() - .min(1, { message: t("please_enter_current_password") }), + .min(1, { message: passwordMessages.current }), // Apply similar changes to other validation messages })Likely invalid or redundant comment.
154-180: 🛠️ Refactor suggestionRemove duplicate validation logic.
The password requirements are already defined in the zod schema. The manual validation here duplicates that logic and could become inconsistent.
- {isPasswordFieldFocused && ( - <div - className="text-small mt-2 pl-2 text-secondary-500" - aria-live="polite" - > - {validateRule( - field.value.length >= 8, - t("password_length_validation"), - !field.value, - )} - {validateRule( - /[a-z]/.test(field.value), - t("password_lowercase_validation"), - !field.value, - )} - {validateRule( - /[A-Z]/.test(field.value), - t("password_uppercase_validation"), - !field.value, - )} - {validateRule( - /\d/.test(field.value), - t("password_number_validation"), - !field.value, - )} - </div> - )} + <FormMessage />Likely invalid or redundant comment.
@rithviknishad done |
|
it doesn't inform the user what the error is when i click submit.
|
rithviknishad
removed
Deploy-Failed
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/components/Users/UserResetPassword.tsx (4)
27-27: Remove unnecessary importThe
mutateimport is redundant since you're usinguseMutationfrom@tanstack/react-query. The mutation function can be defined inline.-import mutate from "@/Utils/request/mutate";
65-65: Remove redundant stateThe
isPasswordFieldFocusedstate is used for manual validation display, which is redundant since we're using zod schema validation. The validation messages should be handled byFormMessagecomponent.
75-81: Simplify mutation setupThe mutation can be simplified by defining the mutation function inline:
const { mutate: resetPassword, isPending } = useMutation({ - mutationFn: mutate(routes.updatePassword), + mutationFn: (data: UpdatePasswordForm) => + routes.updatePassword(data), onSuccess: () => { toast.success(t("password_updated")); form.reset(); }, });
151-154: Simplify error message checkUse optional chaining to simplify the error message check:
- {form.formState.errors?.new_password_1?.message && - form.formState.errors?.new_password_1?.message.includes( - t("new_password_same_as_old"), - ) && <FormMessage />} + {form.formState.errors.new_password_1?.message?.includes( + t("new_password_same_as_old") + ) && <FormMessage />}🧰 Tools
🪛 Biome (1.9.4)
[error] 151-154: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(3 hunks)src/components/Form/Form.tsx(0 hunks)src/components/Users/UserResetPassword.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/Form/Form.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Users/UserResetPassword.tsx
[error] 151-154: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
src/components/Users/UserResetPassword.tsx (2)
83-91: LGTM!The form submission handler is well-implemented with proper typing and data transformation.
155-181: 🛠️ Refactor suggestionRemove duplicate validation display
The manual validation display using
validateRuleduplicates the validation logic already defined in the zod schema. Remove this section and rely onFormMessageto display validation errors.- {isPasswordFieldFocused && ( - <div - className="text-small mt-2 pl-2 text-secondary-500" - aria-live="polite" - > - {validateRule( - field.value.length >= 8, - t("password_length_validation"), - !field.value, - )} - {validateRule( - /[a-z]/.test(field.value), - t("password_lowercase_validation"), - !field.value, - )} - {validateRule( - /[A-Z]/.test(field.value), - t("password_uppercase_validation"), - !field.value, - )} - {validateRule( - /\d/.test(field.value), - t("password_number_validation"), - !field.value, - )} - </div> - )}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/components/ui/input.tsx (3)
11-13: Simplify conditional type retrieval.
Although this function is quite short, consider inlining this logic (e.g.,(type === "password" ? (showPassword ? "text" : "password") : type)) if you prefer brevity. This is optional and mostly a stylistic choice.
16-17: Consider adding an accessible label for the input.
Since theInputcomponent is generic, you might include anaria-labelor alternative text for screen readers if it's used for password inputs. This ensures robust accessibility if the consumer does not provide its own label.Also applies to: 18-34
35-42: Add ARIA attributes to the toggle button for accessibility.
Example:aria-pressed={showPassword}oraria-label="Toggle password visibility". This helps screen readers clarify the toggle’s purpose.<button type="button" + aria-label="Toggle password visibility" ... > <CareIcon icon={showPassword ? "l-eye" : "l-eye-slash"} /> </button>src/components/Users/UserResetPassword.tsx (3)
30-56: Consider movingt()calls outside schema definitions.
If the language changes at runtime, these translations may not re-evaluate, leading to stale strings. Storing the messages in constants or passing them in as parameters can future-proof your localization.- ... .min(1, { message: t("please_enter_current_password") }) ... + const messages = { + pleaseEnterCurrentPassword: t("please_enter_current_password"), + ... + }; const PasswordSchema = z.object({ old_password: z .string() - .min(1, { message: t("please_enter_current_password") }), + .min(1, { message: messages.pleaseEnterCurrentPassword }), ... })
75-81: Leverage global error handling for failures.
You are usingonSuccesswith a success toast here. For errors, react-query’s global error handler can suffice to reduce duplication, though the success toast usage is conventional.
186-214: Properly disable the submit button.
Disabling the button when the form is pristine (!form.formState.isDirty) is a valid approach. If you want to track actual validation readiness, considerisValidfrom form state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Users/UserResetPassword.tsx(1 hunks)src/components/ui/input.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Users/UserResetPassword.tsx
[error] 151-154: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
src/components/ui/input.tsx (2)
5-5: Import looks good!
No issues with introducingCareIconhere.
9-10: Stateful toggle approach is valid.
UsingshowPasswordto manage password visibility is a clean, intuitive solution.src/components/Users/UserResetPassword.tsx (6)
1-2: Imports and initial setup confirmed.
All newly introduced imports (zodResolver,useMutation,z, as well as the updated references toreact-hook-formand new UI components) align well with the shift to schema-based validation and modern form libraries.Also applies to: 5-5, 7-8, 13-21, 27-27
65-66: Avoid maintaining extra focus states.
isPasswordFieldFocusedis used only to toggle manual validations. This partially duplicates the Zod validations. Consider removing the manual validations and relying fully on the form state.
67-74: Initial form setup looks good.
Assigning default values and applying thezodResolver(PasswordSchema)is consistent with best practices.
83-85: Submission logic is concise.
The approach correctly mergesold_password,username, and the new password. Ensure that the backend route expects these fields exactly.Also applies to: 91-91
94-107: Toggle logic for password changes is clean.
Unifying theisEditingstate to show/hide the form is straightforward. The new fields and the structure introduced by theFormandFormFieldcomponents help maintain clarity.Also applies to: 108-132, 133-154
155-181: Avoid duplicating Zod validations in UI.
Zod schema validations already cover length, lowercase, uppercase, and numeric checks. This manual check may drift out of sync.
…thub.com/AdityaJ2305/care_fe into issue/9683/userResetPassword_form_switch
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Users/UserResetPassword.tsx (1)
67-81: Simplify form name and use response message
- Since this is the only form in the file, the form name can be simplified
- Use the success message from the API response
- const form = useForm({ + const form = useForm<z.infer<typeof PasswordSchema>>({ resolver: zodResolver(PasswordSchema), defaultValues: { old_password: "", new_password_1: "", new_password_2: "", }, }); const { mutate: resetPassword, isPending } = useMutation({ mutationFn: mutate(routes.updatePassword), - onSuccess: () => { - toast.success(t("password_updated")); + onSuccess: (data) => { + toast.success(data?.message ?? t("password_updated")); form.reset(); }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(4 hunks)src/components/Users/UserResetPassword.tsx(1 hunks)
🔇 Additional comments (4)
src/components/Users/UserResetPassword.tsx (3)
207-214: LGTM! Button implementation is correctThe button correctly:
- Uses isPending for loading state
- Shows appropriate text based on submission state
- Disables when form is not dirty
151-182: 🛠️ Refactor suggestionRemove duplicate validation logic
The password requirements are already defined in the zod schema. The manual validation here duplicates that logic and could become inconsistent.
Use the form's error state instead:
- {isPasswordFieldFocused && ( - <div - className="text-small mt-2 pl-2 text-secondary-500" - aria-live="polite" - > - {validateRule( - field.value.length >= 8, - t("password_length_validation"), - !field.value, - )} - {validateRule( - /[a-z]/.test(field.value), - t("password_lowercase_validation"), - !field.value, - )} - {validateRule( - /[A-Z]/.test(field.value), - t("password_uppercase_validation"), - !field.value, - )} - {validateRule( - /\d/.test(field.value), - t("password_number_validation"), - !field.value, - )} - {validateRule( - field.value !== form.watch("old_password"), - t("new_password_same_as_old"), - !field.value, - )} - </div> - )} + <FormMessage />Likely invalid or redundant comment.
30-56: 🛠️ Refactor suggestionExtract translation calls from schema definition
The t() function calls within the schema definition might not work correctly with dynamic language changes since they're evaluated only once when the schema is defined.
Extract the messages to constants:
+ const messages = { + currentPassword: t("please_enter_current_password"), + passwordLength: t("password_length_validation"), + passwordNumber: t("password_number_validation"), + passwordLowercase: t("password_lowercase_validation"), + passwordUppercase: t("password_uppercase_validation"), + confirmPassword: t("please_enter_confirm_password"), + passwordMismatch: t("password_mismatch"), + passwordSameAsOld: t("new_password_same_as_old"), + }; const PasswordSchema = z .object({ old_password: z .string() - .min(1, { message: t("please_enter_current_password") }), + .min(1, { message: messages.currentPassword }), // Apply similar changes to other validation messages })Likely invalid or redundant comment.
public/locale/en.json (1)
1203-1203: LGTM! Translation changes are consistentThe changes to the translation keys align with the form implementation and provide clear user feedback.
Also applies to: 1286-1286, 1326-1326, 1402-1402
There was a problem hiding this comment.
do not modify shadcn's input component. you may extend it by creating a new component instead.
There was a problem hiding this comment.
also why was the login page not updated like mentioned in the issue?
also update in the user create form
Proposed Changes
UserResetPassword.tsxto use shadcn's form along withuseForm#9683@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
Removed Components
Formcomponent and related context managementExtendPatientRegisterFormComponentTypeNew Features
Localization
Improvements