-
Notifications
You must be signed in to change notification settings - Fork 516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Switch to shadcn form in userResetPassword #9708
Fix: Switch to shadcn form in userResetPassword #9708
Conversation
WalkthroughThis pull request introduces significant changes to form handling, password management, and user interface components. The primary modifications include removing the custom Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you missed one step, one more file was said to be deleted in the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 bothzod
to validate passwords and thevalidateRule
function 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 withsetIsEditing
is a clean, straightforward approach and helps keep the UI clear. No issues found here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
isPending
fromuseMutation
instead 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_password
while 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
isDirty
for the disabled stateisPending
for 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.tsx
already usesold_password
in:
- 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repeating the second time, do delete the other file that was mentioned in the issue
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
|
yup, you can remove that patient register form extension thing.... Lines 30 to 43 in b36f39f
cc @gigincg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/components/Auth/ResetPassword.tsx (1)
Missing translation key in password reset form
The translation key
password_match
is missing from en.json. This key is used to show feedback when passwords match in the confirm password field.🔗 Analysis chain
Line range hint
1-211
: Verify I18n text completenessAs mentioned in PR comments, ensure all translation keys are present in the locale files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Extract all t() calls and verify their presence in en.json # Extract all translation keys used in the file echo "Checking translation keys..." KEYS=$(rg "t\([\"'](.*?)[\"']\)" -o --replace '$1' src/components/Auth/ResetPassword.tsx) # Check if each key exists in en.json echo "Found keys:" echo "$KEYS" echo "Verifying presence in en.json..." for key in $KEYS; do rg "\"$key\":" public/locale/en.json || echo "Missing translation for: $key" doneLength of output: 3788
♻️ Duplicate comments (1)
src/Utils/request/errorHandler.ts (1)
88-99
: 🛠️ Refactor suggestionDisplay all error messages instead of returning early.
The current implementation only shows the first encountered error message due to early returns. Users should see all error messages to address multiple issues at once.
function handleStructuredErrors(cause: StructuredError) { for (const value of Object.values(cause)) { if (Array.isArray(value)) { value.forEach((err) => toast.error(err)); - return; } if (typeof value === "string") { toast.error(value); - return; } } }
🧹 Nitpick comments (2)
src/Utils/request/errorHandler.ts (1)
84-86
: Enhance type safety with a more specific return type.The type guard can be more precise by explicitly defining the return type.
-function isStructuredError(err: HTTPError["cause"]): err is StructuredError { +function isStructuredError(err: HTTPError["cause"]): err is Record<string, string | string[]> { return typeof err === "object" && !Array.isArray(err); }src/components/Auth/ResetPassword.tsx (1)
Line range hint
36-43
: Consider simplifying the handleChange implementationThe current implementation can be made more concise while maintaining the same functionality.
-const handleChange = (e: any) => { - const { value, name } = e.target; - const fieldValue = Object.assign({}, form); - const errorField = Object.assign({}, errors); - if (errorField[name]) { - errorField[name] = null; - setErrors(errorField); - } - fieldValue[name] = value; - setForm(fieldValue); +const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { + const { value, name } = e.target; + setForm(prev => ({ ...prev, [name]: value })); + setErrors(prev => ({ ...prev, [name]: null })); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
public/locale/en.json
(7 hunks)src/Utils/request/errorHandler.ts
(4 hunks)src/components/Auth/Login.tsx
(2 hunks)src/components/Auth/ResetPassword.tsx
(3 hunks)src/components/Users/CreateUserForm.tsx
(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/Users/CreateUserForm.tsx
- src/components/Auth/Login.tsx
- public/locale/en.json
🔇 Additional comments (4)
src/Utils/request/errorHandler.ts (3)
78-81
: LGTM! Type definitions align with backend error structure.The optional properties in
PydanticError
type correctly reflect thatloc
,input
, andurl
may not always be present in backend responses.
112-117
: LGTM! Robust error message handling.The implementation correctly handles:
- Both string and object message types
- Cases where location is undefined
- Proper error message formatting with type and location
Also applies to: 120-122
40-45
: LGTM! Clean integration of structured error handling.The structured error check and handling is well-integrated into the existing error handling flow, maintaining consistency with the established pattern.
src/components/Auth/ResetPassword.tsx (1)
7-7
: LGTM: Successful migration to shadcn form componentsThe switch to
PasswordInput
from the UI component library aligns with the modernization goals.
👋 Hi, @AdityaJ2305, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, some minor things. let's get it merged after that
@rithviknishad done with the minor changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Users/CreateUserForm.tsx (2)
49-90
: Consider enhancing password validation and adding type documentation.The validation schema is well-structured, but consider these improvements:
- Add special character requirement for passwords
- Add JSDoc comments to document the schema type
+ /** + * Schema for user creation form + * @property {string} user_type - Type of user (doctor, nurse, staff, volunteer) + * @property {string} username - Unique username (4-16 chars) + * @property {string} password - Strong password with specific requirements + * ...other properties + */ const userFormSchema = z .object({ // ... other fields password: z .string() .min(8, t("password_length_validation")) .regex(/[a-z]/, t("password_lowercase_validation")) .regex(/[A-Z]/, t("password_uppercase_validation")) .regex(/[0-9]/, t("password_number_validation")) + .regex(/[!@#$%^&*(),.?":{}|<>]/, t("password_special_char_validation")),
132-140
: Use more descriptive variable naming.
isInitialRender
could be renamed to better reflect its purpose.- const isInitialRender = usernameInput === ""; + const isUsernameEmpty = usernameInput === "";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json
(11 hunks)src/components/Users/CreateUserForm.tsx
(7 hunks)src/components/ui/input-password.tsx
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/ui/input-password.tsx
- public/locale/en.json
🔇 Additional comments (3)
src/components/Users/CreateUserForm.tsx (3)
22-22
: LGTM! The PasswordInput component import is correctly placed with other UI imports.
119-119
: Simplify query destructuring.Since this is not the primary API of this component, consider skipping the destructuring or using an alias.
- const { isLoading: isUsernameChecking, isError: isUsernameTaken } = useQuery({ + const usernameCheckQuery = useQuery({
Line range hint
268-285
: Ensure PasswordInput component meets accessibility requirements.The PasswordInput component should include proper accessibility attributes:
- Add
aria-label
for the visibility toggle button- Include
aria-pressed
state- Add
aria-controls
for input association- Maintain visible focus indication
#!/bin/bash # Verify accessibility attributes in PasswordInput component ast-grep --pattern 'export function PasswordInput' -A 20
for required fields, we reuse "field_required" instead of creating very specific translations for each field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
public/locale/en.json (1)
2002-2009
: Consider consistent phrasing for username validation messages.While the messages are clear, consider rephrasing for consistency with the positive language pattern used in password validation:
- "username_characters_validation": "Only lowercase letters, numbers, and . _ - are allowed", + "username_characters_validation": "Use lowercase letters, numbers, and . _ - only", - "username_consecutive_validation": "Cannot contain consecutive special characters", + "username_consecutive_validation": "Use non-consecutive special characters", - "username_start_end_validation": "Must start and end with a letter or number", + "username_start_end_validation": "Start and end with a letter or number"src/components/Users/CreateUserForm.tsx (1)
49-90
: Consider enhancing password validation rules.While the current password validation is good, consider adding these common security requirements:
- Special characters requirement
- Maximum length limit
- Prevention of common patterns
password: z .string() .min(8, t("password_length_validation")) + .max(128, t("password_max_length_validation")) .regex(/[a-z]/, t("password_lowercase_validation")) .regex(/[A-Z]/, t("password_uppercase_validation")) .regex(/[0-9]/, t("password_number_validation")) + .regex(/[!@#$%^&*(),.?":{}|<>]/, t("password_special_char_validation")) + .refine( + (val) => !/(123|abc|password|admin)/i.test(val), + t("password_common_pattern_validation") + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json
(9 hunks)src/components/Users/CreateUserForm.tsx
(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (6)
public/locale/en.json (2)
1413-1419
: LGTM! Clear and consistent password validation messages.The password validation messages follow best practices:
- Use positive, action-oriented language
- Provide clear success indicators
- Maintain consistent structure
Also applies to: 1426-1428
1283-1284
: LGTM! Clear password change validation messages.The messages effectively communicate password change requirements while maintaining security best practices.
src/components/Users/CreateUserForm.tsx (4)
22-22
: LGTM!The import follows the project's conventions and is correctly placed with other UI component imports.
119-119
: Consider aliasing the destructured query properties.While the change from
error
toisError
is correct for tanstack-query v5, consider aliasing the destructured properties for better clarity:- const { isLoading: isUsernameChecking, isError: isUsernameTaken } = useQuery({ + const { + isLoading: isUsernameChecking, + isError: isUsernameTaken + } = useQuery({
Line range hint
132-159
: LGTM! Improved validation UX.The addition of
isInitialRender
and updated validation logic improves the user experience by preventing premature validation feedback.
268-268
: Verify accessibility improvements in PasswordInput component.Please ensure that the PasswordInput component addresses these accessibility requirements:
- ARIA labels for the visibility toggle
- Keyboard navigation support
- Visible focus indicators
#!/bin/bash # Check if PasswordInput component has accessibility attributes echo "=== Checking PasswordInput component for accessibility attributes ===" ast-grep --pattern 'export function PasswordInput' -A 30 echo -e "\n=== Checking for ARIA attributes ===" rg -i "aria-|role=" src/components/ui/input-password.tsxAlso applies to: 282-285
@AdityaJ2305 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
UserResetPassword.tsx
to use shadcn's form along withuseForm
#9683@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
PasswordInput
component with password visibility toggle.Improvements
Input
component's onFocus behavior.Bug Fixes
Chores
FormContext
and related types.Form
component from the application.ExtendPatientRegisterForm
component type from supported plugins.