Edit User Details #10027
Edit User Details #10027
Conversation
WalkthroughThis pull request introduces comprehensive enhancements to user management functionality across multiple components. The changes include adding localization support for user-related actions, creating a new 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: 4
🔭 Outside diff range comments (1)
src/components/Users/UserForm.tsx (1)
Line range hint
53-109: Review conditional schema for consistency and completenessThe form schema conditionally makes several fields optional based on
isEditMode. While this allows flexibility, please verify:
- All required fields are enforced correctly in both modes.
- Optional fields in edit mode still validate correctly if provided.
- Conditional logic does not introduce unintended side effects.
🧹 Nitpick comments (12)
src/components/Users/UserForm.tsx (8)
2-2: Consider removing unused importuseQueryThe
useQueryimport from@tanstack/react-queryis not used in this section of the code. Removing unused imports can help keep the code clean and improve readability.-import { useMutation, useQuery } from "@tanstack/react-query"; +import { useMutation } from "@tanstack/react-query";
124-130: Handle potential query issues when fetching user dataThe
useQueryhook depends onexistingUsername. Ensure thatexistingUsernameis defined before initiating the query to prevent unnecessary network requests or errors.Consider adding a check to prevent the query from running if
existingUsernameis undefined or empty.
132-146: Remove debug statements from production codeThe
console.log(userData);statement is useful for debugging but should be removed to keep the console clean in production.- console.log(userData);
157-160: Optimize form triggering for better performanceThe
form.trigger("username");is called inside auseEffectwithout specifying dependencies strictly. This could lead to unnecessary re-renders or validations.Ensure that
form.trigger("username");is only called when necessary, possibly by adjusting the dependencies of theuseEffect.
206-216: Enhance error handling for user creationThe
createUsermutation lacks detailed error handling. Consider capturing specific error messages from the API to provide more informative feedback to the user.You could access
error.response.dataif available to display validation errors returned from the server.
218-230: Implement comprehensive error handling for user updatesSimilarly, the
updateUsermutation could benefit from enhanced error handling to provide users with specific feedback when an update fails.
246-247: Remove leftover console logsThere is a
console.log(form.formState.errors);statement that should be removed to clean up the code.- console.log(form.formState.errors);
318-369: Confirm password update flowPassword fields are hidden in edit mode, meaning users cannot change their passwords through this form. If password updates are required, consider adding functionality to handle password changes securely.
If implementing password updates, ensure that current password verification and appropriate security measures are in place.
src/pages/Organization/components/EditUserSheet.tsx (2)
15-20: Props interface looks good, but consider adding validation props.The interface is well-structured. Consider adding validation props to handle cases where editing might be restricted.
interface EditUserSheetProps { existingUsername: string; open: boolean; setOpen: (open: boolean) => void; onUserUpdated?: (user: UserBase) => void; + isEditable?: boolean; + validationError?: string; }
31-34: Add ARIA labels for better accessibility.The sheet content should have appropriate ARIA labels for screen readers.
<SheetContent className="w-full sm:max-w-2xl overflow-y-auto" data-cy="add-user-form" + aria-label={t("edit_user_form")} >src/components/Users/UserSummary.tsx (1)
110-120: Consider adding loading state to the edit button.The edit button should reflect loading state during user updates.
<Button variant="outline" className="w-fit self-end" data-cy="edit-user-button" + disabled={isUpdating} onClick={() => setShowEditUserSheet(true)} > <CareIcon icon="l-pen" className="mr-2 h-4 w-4" /> - {t("edit_user")} + {isUpdating ? t("updating") : t("edit_user")} </Button>src/pages/Organization/components/EditUserRoleSheet.tsx (1)
249-258: Move edit button next to user info for better UX.Consider relocating the edit button to be closer to the user information section for improved user experience.
The edit button would be more intuitive if placed near the user's details (around line 154) rather than at the bottom of the action buttons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
public/locale/en.json(5 hunks)src/common/constants.tsx(1 hunks)src/components/Patient/PatientRegistration.tsx(1 hunks)src/components/Users/UserForm.tsx(9 hunks)src/components/Users/UserSummary.tsx(3 hunks)src/pages/Organization/components/AddUserSheet.tsx(3 hunks)src/pages/Organization/components/EditUserRoleSheet.tsx(3 hunks)src/pages/Organization/components/EditUserSheet.tsx(1 hunks)src/types/user/user.ts(1 hunks)src/types/user/userApi.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Users/UserForm.tsx (1)
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 (18)
src/components/Users/UserForm.tsx (7)
44-44: EnsureexistingUsernameis passed as a prop when in edit modeThe
existingUsernameprop is added to determine edit mode. Verify that this prop is correctly passed from the parent component when editing a user to prevent unexpected behavior.
49-49: Confirm the reliability of theisEditModecheckThe
isEditModeflag uses double negation onexistingUsername. Ensure thatexistingUsernameis reliably defined (not an empty string or falsy value) when in edit mode to prevent logic errors.
94-96: Re-evaluate optionalgeo_organizationfield in edit modeThe
geo_organizationfield is optional in edit mode. Confirm if the organization should be editable or required when updating a user, as changing a user's organization might have significant implications.
168-168: Prevent username availability check in edit modeThe username availability check should not run in edit mode, as the username is not intended to be changed. The
enabledcondition already accounts for this, but double-check to ensure it's functioning as expected.
251-278: Review conditional rendering of user type selectionThe user type selection is hidden in edit mode. Ensure that this aligns with the application's requirements. If user roles should be editable, consider displaying this field when appropriate permissions are granted.
552-568: Validate the necessity ofgeo_organizationin edit modeThe
geo_organizationfield is conditionally rendered only in create mode. Confirm whether users should be able to change their organization during an edit. If so, adjust the form to allow editing this field.
571-573: Button label correctly reflects the form modeThe submit button label changes based on
isEditMode, which enhances user experience by providing appropriate context.src/types/user/userApi.ts (2)
15-15: UpdateTBodyto useCreateUserModelChanging
TBodytoCreateUserModelin thecreatemethod ensures that the correct data structure is used when creating a user.
27-32: Addupdatemethod touserApiThe
updatemethod allows for updating user data via a PUT request. Ensure that the backend supports this endpoint and that it properly handles partial updates.Double-check if
Partial<UpdateUserModel>aligns with the backend expectations. If the backend supports PATCH requests for partial updates, consider usingHttpMethod.PATCHinstead.src/types/user/user.ts (2)
18-33: DefineCreateUserModelfor user creationThe
CreateUserModeltype includes all necessary fields for creating a new user, which improves type safety and clarity.
35-35: Review fields excluded inUpdateUserModelThe
UpdateUserModelomitsusernameandpassword, indicating these fields are not updatable. Verify if users should be able to change their passwords or usernames, and adjust the model accordingly.src/pages/Organization/components/AddUserSheet.tsx (2)
44-45: LGTM! Good use of translations.The translation keys are descriptive and follow the established pattern.
Line range hint
48-53: Verify UserForm compatibility.The switch from CreateUserForm to UserForm needs verification to ensure all creation-specific functionality is preserved.
✅ Verification successful
The switch to UserForm is compatible and safe
The UserForm component is a unified implementation that handles both user creation and editing. When used without the
existingUsernameprop (as in AddUserSheet), it operates in creation mode with all the necessary functionality preserved.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if UserForm maintains all props from CreateUserForm ast-grep --pattern 'interface $interface { $$$ }' | grep -A 10 'CreateUserForm|UserForm'Length of output: 92
Script:
#!/bin/bash # Find both component definitions and their types echo "=== Searching for UserForm ===" rg "UserForm" -A 5 -B 5 echo -e "\n=== Searching for CreateUserForm ===" rg "CreateUserForm" -A 5 -B 5 echo -e "\n=== Looking for type definitions ===" ast-grep --pattern 'type $name = { $$$ }' ast-grep --pattern 'export const $name: FC<{ $$$ }>'Length of output: 12532
src/components/Users/UserSummary.tsx (1)
103-108: LGTM! Clean integration of EditUserSheet.The component integration is well-structured and maintains proper prop passing.
src/pages/Organization/components/EditUserRoleSheet.tsx (1)
124-125: LGTM! Good permission handling.The use of useAuthUser and editUserPermissions follows best practices for permission management.
src/common/constants.tsx (1)
107-109: LGTM! Well-implemented constant definition.The implementation follows best practices by:
- Maintaining type safety with proper TypeScript types
- Reusing the existing
GENDER_TYPESconstant- Following the established pattern in the codebase
src/components/Patient/PatientRegistration.tsx (1)
48-48: LGTM! Good refactoring to use centralized constant.The change improves maintainability by using the centralized
GENDERSconstant instead of a local definition.public/locale/en.json (1)
352-352: LGTM! Well-structured localization additions.The new localization keys follow consistent naming patterns and provide clear, descriptive messages for user management functionality.
Also applies to: 702-702, 830-830, 2114-2114, 2152-2152
| if (isEditMode) { | ||
| updateUser({ | ||
| ...data, | ||
| } as UpdateUserModel); | ||
| } else { |
There was a problem hiding this comment.
Ensure correct data is sent in the update request
When updating a user, all fields from data are being sent. If the API expects only changed fields, consider sending only the modified fields to prevent unintended data overwrites.
Consider using diff or selectively including fields:
const updatedFields = { /* include only the fields that have changed */ };
updateUser(updatedFields as UpdateUserModel);
|
👋 Hi, @Jacobjeevan, 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 (3)
src/components/Users/UserForm.tsx (3)
124-146: Remove debug console.log statement.The user data fetching and form initialization look good, but there's a debug console.log statement that should be removed.
- console.log(userData);
233-237: Consider sending only modified fields during update.The current implementation sends all form fields during update, which could lead to unintended overwrites. Consider tracking and sending only the modified fields.
const getModifiedFields = (data: UserFormValues, originalData: UserBase) => { const modifiedFields: Partial<UserFormValues> = {}; Object.keys(data).forEach((key) => { if (data[key] !== originalData[key]) { modifiedFields[key] = data[key]; } }); return modifiedFields; };
246-246: Remove debug console.log statement.Remove the debug console.log statement for form errors.
- console.log(form.formState.errors);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(5 hunks)src/components/Patient/PatientRegistration.tsx(1 hunks)src/components/Users/UserForm.tsx(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🧰 Additional context used
📓 Learnings (1)
src/components/Users/UserForm.tsx (2)
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.
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#10027
File: src/components/Users/UserForm.tsx:34-34
Timestamp: 2025-01-17T11:34:37.003Z
Learning: In the care_fe codebase, the GENDERS constant is defined with a type assertion `as [(typeof GENDER_TYPES)[number]["id"]]` which makes it compatible with `z.enum()` in Zod schemas. This pattern is preferred over using string literals directly.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (5)
src/components/Users/UserForm.tsx (4)
2-2: LGTM!The imports and type definitions are well-structured and support the form's dual functionality for creating and editing users.
Also applies to: 34-34, 44-44
53-76: LGTM!The form schema effectively handles both create and edit modes with appropriate conditional validation. The password and username fields are correctly made optional during editing.
98-109: LGTM!The password confirmation validation is correctly implemented to only apply during user creation.
251-278: LGTM!The form rendering logic effectively handles both create and edit modes with proper conditional rendering and translations.
Also applies to: 318-369, 552-568
src/components/Patient/PatientRegistration.tsx (1)
48-48: LGTM!The import of the centralized
GENDERSconstant aligns with the codebase's move towards consistent gender option management.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Patient/PatientRegistration.tsx (1)
Line range hint
33-38: Consider adding error boundary for error handling.The error handling for mutations could be improved by adding an error boundary component to gracefully handle and display errors to users.
+ import ErrorBoundary from '@/components/Common/ErrorBoundary'; export default function PatientRegistration(props: PatientRegistrationPageProps) { + return ( + <ErrorBoundary> {/* existing component code */} + </ErrorBoundary> + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(5 hunks)src/components/Patient/PatientRegistration.tsx(1 hunks)src/components/Users/UserSummary.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Users/UserSummary.tsx
🔇 Additional comments (3)
src/components/Patient/PatientRegistration.tsx (2)
49-49: LGTM! Good refactoring of GENDERS constant.Moving the GENDERS constant to common constants improves code reusability and maintainability.
Line range hint
9-10: Verify toast notification accessibility.Ensure toast notifications are accessible by adding proper ARIA attributes and role.
✅ Verification successful
Toast notifications are properly accessible ✅
The toast notifications are implemented using Radix UI's toast primitives, which provide comprehensive accessibility features including:
- Proper ARIA attributes and roles
- Keyboard navigation support
- Focus management
- Semantic structure with title and description
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if toast notifications have proper ARIA attributes rg -A 2 'toast\.' src/components/Patient/PatientRegistration.tsxLength of output: 488
Script:
#!/bin/bash # Search for toast-related imports and definitions echo "=== Toast imports ===" rg "import.*toast" -i --type ts --type tsx echo -e "\n=== Toast component definitions ===" ast-grep --pattern 'const Toast = $_' echo -e "\n=== Toast configurations ===" rg "toast\.(configure|setup)" -i --type ts --type tsxLength of output: 1189
Script:
#!/bin/bash # Check the toast component implementation echo "=== Toast Component Implementation ===" cat src/components/ui/toast.tsxLength of output: 5562
public/locale/en.json (1)
352-352: LGTM! Translations added for user management.The new translations are well-structured and consistent with the existing format:
- add_user
- create_user_and_add_to_org
- edit_user
- update_user
- user_updated_successfully
Also applies to: 704-704, 834-834, 2131-2131, 2169-2169
|
Conditionally approving it, you can take rest of them as a separate PR
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/components/Users/UserForm.tsx (6)
71-75: Consider strengthening password validation.The current password validation could be enhanced for better security:
- Add special character requirement
- Increase minimum length to 12 characters
- Add maximum length constraint
.string() -.min(8, t("password_length_validation")) +.min(12, 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(/[0-9]/, t("password_number_validation")) +.regex(/[!@#$%^&*(),.?":{}|<>]/, t("password_special_char_validation")),
Line range hint
82-86: Consider making phone number validation more flexible.The current validation only accepts Indian phone numbers (+91). Consider making it more flexible to support international numbers or configurable based on deployment region.
phone_number: z .string() - .regex(/^\+91[0-9]{10}$/, t("phone_number_validation")), + .regex(/^\+[1-9]\d{1,14}$/, t("phone_number_validation")), // E.164 format alt_phone_number: z .string() - .regex(/^\+91[0-9]{10}$/, t("phone_number_validation")) + .regex(/^\+[1-9]\d{1,14}$/, t("phone_number_validation")) // E.164 format
89-93: Track TODOs in the issue system.The TODO comments about additional fields (qualification, experience, registration) should be tracked in the issue system for better visibility and follow-up.
Would you like me to create an issue to track these pending fields? I can help draft the issue with the requirements and implementation details.
134-134: Remove debug console.log statement.Remove the debug logging statement before merging to production.
- console.log(userData);
233-237: Optimize update payload.The current implementation sends all form data to the update endpoint. Consider sending only the modified fields to optimize the request payload and prevent unnecessary updates.
- updateUser({ - ...data, - } as UpdateUserModel); + const dirtyFields = Object.keys(form.formState.dirtyFields); + const changedData = dirtyFields.reduce((acc, key) => ({ + ...acc, + [key]: data[key as keyof UserFormValues], + }), {}); + updateUser(changedData as UpdateUserModel);
246-246: Remove debug console.log statement.Remove the debug logging statement before merging to production.
- console.log(form.formState.errors);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cypress/e2e/users_spec/user_creation.cy.ts(1 hunks)cypress/pageObject/Users/UserCreation.ts(0 hunks)src/components/Users/UserForm.tsx(9 hunks)
💤 Files with no reviewable changes (1)
- cypress/pageObject/Users/UserCreation.ts
✅ Files skipped from review due to trivial changes (1)
- cypress/e2e/users_spec/user_creation.cy.ts
🧰 Additional context used
📓 Learnings (1)
src/components/Users/UserForm.tsx (2)
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.
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#10027
File: src/components/Users/UserForm.tsx:34-34
Timestamp: 2025-01-17T11:34:37.003Z
Learning: In the care_fe codebase, the GENDERS constant is defined with a type assertion `as [(typeof GENDER_TYPES)[number]["id"]]` which makes it compatible with `z.enum()` in Zod schemas. This pattern is preferred over using string literals directly.
🔇 Additional comments (3)
src/components/Users/UserForm.tsx (3)
Line range hint
1-46: LGTM! Clean imports and well-structured type definitions.The imports are organized logically, and the Props interface is properly typed with the new optional
existingUsernameparameter.
Line range hint
148-205: LGTM! Well-implemented form effects and validation.The username validation with availability check and WhatsApp number synchronization are implemented correctly with proper error handling and loading states.
Line range hint
251-587: LGTM! Well-structured form rendering.The form rendering is well-organized with:
- Proper conditional rendering based on edit mode
- Clear separation of form sections
- Consistent use of form controls and validation messages
- Appropriate data-cy attributes for testing
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/components/Users/UserForm.tsx (5)
39-39: Consider using type assertions more effectively.The imported types
CreateUserModelandUpdateUserModelare used in type assertions, but the spread operator might not guarantee type safety. Consider explicitly mapping the form data to these types.- updateUser({ - ...data, - } as UpdateUserModel); + const updateData: UpdateUserModel = { + first_name: data.first_name, + last_name: data.last_name, + email: data.email, + phone_number: data.phone_number, + gender: data.gender, + alt_phone_number: data.alt_phone_number, + phone_number_is_whatsapp: data.phone_number_is_whatsapp, + }; + updateUser(updateData);Also applies to: 231-240
89-93: Plan implementation of additional user details.The TODO comment indicates pending support for additional user details (qualification, experience, registration). Consider creating separate issues to track the implementation of these fields.
Would you like me to create issues for implementing:
- Backend support for additional user details
- Frontend integration of these fields
- Database schema updates
Line range hint
146-158: Optimize effect dependencies.The effect has multiple dependencies and responsibilities. Consider splitting it into separate effects for username validation and phone number synchronization.
- useEffect(() => { - if (isWhatsApp) { - form.setValue("alt_phone_number", phoneNumber); - } - if (usernameInput && usernameInput.length > 0 && !isEditMode) { - form.trigger("username"); - } - }, [phoneNumber, isWhatsApp, form, usernameInput, isEditMode]); + useEffect(() => { + if (isWhatsApp) { + form.setValue("alt_phone_number", phoneNumber); + } + }, [phoneNumber, isWhatsApp, form]); + + useEffect(() => { + if (usernameInput && usernameInput.length > 0 && !isEditMode) { + form.trigger("username"); + } + }, [usernameInput, isEditMode, form]);
216-228: Consider implementing optimistic updates.The update mutation could benefit from optimistic updates to improve the user experience. This would make the UI feel more responsive by immediately showing the changes while the backend request is in progress.
const { mutate: updateUser, isPending: updatePending } = useMutation({ mutationKey: ["update_user"], mutationFn: mutate(userApi.update, { pathParams: { username: existingUsername! }, }), onMutate: async (newData) => { // Cancel outgoing refetches await queryClient.cancelQueries({ queryKey: ["user", existingUsername] }); // Snapshot the previous value const previousData = queryClient.getQueryData(["user", existingUsername]); // Optimistically update queryClient.setQueryData(["user", existingUsername], newData); return { previousData }; }, onError: (error, _, context) => { // Rollback on error queryClient.setQueryData(["user", existingUsername], context?.previousData); toast.error(error?.message ?? t("user_update_error")); }, onSuccess: (resp: UserBase) => { toast.success(t("user_updated_successfully")); onSubmitSuccess?.(resp); }, });
577-589: Enhance button accessibility.The submit button could benefit from better accessibility attributes and loading state indication.
<Button type="submit" className="w-full" data-cy="submit-user-form" + aria-busy={updatePending || createPending} + aria-disabled={isLoadingUser || !form.formState.isDirty} disabled={ isLoadingUser || !form.formState.isDirty || updatePending || createPending } > + {(updatePending || createPending) && ( + <CareIcon icon="l-spinner" className="mr-2 h-4 w-4 animate-spin" /> + )} {isEditMode ? t("update_user") : t("create_user")} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/UserForm.tsx(12 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Users/UserForm.tsx (2)
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.
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#10027
File: src/components/Users/UserForm.tsx:34-34
Timestamp: 2025-01-17T11:34:37.003Z
Learning: In the care_fe codebase, the GENDERS constant is defined with a type assertion `as [(typeof GENDER_TYPES)[number]["id"]]` which makes it compatible with `z.enum()` in Zod schemas. This pattern is preferred over using string literals directly.
| useEffect(() => { | ||
| if (userData && isEditMode) { | ||
| const formData: Partial<UserFormValues> = { | ||
| user_type: userData.user_type, | ||
| first_name: userData.first_name, | ||
| last_name: userData.last_name, | ||
| email: userData.email, | ||
| phone_number: userData.phone_number || "", | ||
| gender: userData.gender, | ||
| phone_number_is_whatsapp: true, | ||
| }; | ||
| form.reset(formData); | ||
| } | ||
| }, [userData, form, isEditMode]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance form reset handling.
The form reset doesn't handle all fields consistently. Some fields like alt_phone_number and geo_organization are not included in the reset data, which might lead to unexpected behavior.
const formData: Partial<UserFormValues> = {
user_type: userData.user_type,
first_name: userData.first_name,
last_name: userData.last_name,
email: userData.email,
phone_number: userData.phone_number || "",
gender: userData.gender,
phone_number_is_whatsapp: true,
+ alt_phone_number: userData.alt_phone_number || "+91",
+ geo_organization: userData.geo_organization,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (userData && isEditMode) { | |
| const formData: Partial<UserFormValues> = { | |
| user_type: userData.user_type, | |
| first_name: userData.first_name, | |
| last_name: userData.last_name, | |
| email: userData.email, | |
| phone_number: userData.phone_number || "", | |
| gender: userData.gender, | |
| phone_number_is_whatsapp: true, | |
| }; | |
| form.reset(formData); | |
| } | |
| }, [userData, form, isEditMode]); | |
| useEffect(() => { | |
| if (userData && isEditMode) { | |
| const formData: Partial<UserFormValues> = { | |
| user_type: userData.user_type, | |
| first_name: userData.first_name, | |
| last_name: userData.last_name, | |
| email: userData.email, | |
| phone_number: userData.phone_number || "", | |
| gender: userData.gender, | |
| phone_number_is_whatsapp: true, | |
| alt_phone_number: userData.alt_phone_number || "+91", | |
| geo_organization: userData.geo_organization, | |
| }; | |
| form.reset(formData); | |
| } | |
| }, [userData, form, isEditMode]); |
src/components/Users/UserForm.tsx
Outdated
| const { mutate: updateUser, isPending: updatePending } = useMutation({ | ||
| mutationKey: ["update_user"], | ||
| mutationFn: mutate(userApi.update, { | ||
| pathParams: { username: existingUsername! }, | ||
| }), | ||
| onSuccess: (resp: UserBase) => { | ||
| toast.success(t("user_updated_successfully")); | ||
| onSubmitSuccess?.(resp); | ||
| }, | ||
| onError: (error) => { | ||
| toast.error(error?.message ?? t("user_update_error")); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
we should be invalidating the user list and user retrieve queries here
There was a problem hiding this comment.
Yep, I added the user one based on #10045 (which is converting the user retrieve to using tanstack).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/components/Users/UserForm.tsx (2)
132-145:⚠️ Potential issueInclude all available fields in form reset.
The form reset doesn't include all available fields from userData, which could lead to inconsistent form state.
const formData: Partial<UserFormValues> = { user_type: userData.user_type, first_name: userData.first_name, last_name: userData.last_name, email: userData.email, phone_number: userData.phone_number || "", gender: userData.gender, phone_number_is_whatsapp: true, + alt_phone_number: userData.alt_phone_number || "+91", + geo_organization: userData.geo_organization, };
253-257: 🛠️ Refactor suggestionConsider sending only modified fields in update request.
The update mutation sends all form data, which could lead to unintended updates of unchanged fields.
Consider using a diff to send only modified fields:
const changedFields = Object.keys(form.formState.dirtyFields).reduce( (acc, key) => ({ ...acc, [key]: data[key as keyof UserFormValues], }), {} as Partial<UpdateUserModel> ); updateUser(changedFields);
🧹 Nitpick comments (4)
src/components/Users/UserForm.tsx (4)
2-2: Consider combining related imports.The import from
@tanstack/react-querycan be combined into a single line for better readability.-import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query";
90-94: Track TODO items in issue tracker.These TODO comments about additional user fields should be tracked in the issue system for better visibility and follow-up.
Would you like me to create an issue to track the implementation of these additional fields (qualification, experience, and medical registration)?
69-76: Enhance password validation rules.Consider adding additional password validation rules for special characters and maximum length to improve security.
password: isEditMode ? z.string().optional() : 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")),
Line range hint
486-507: Improve gender field layout.The gender field is placed in a grid with 2 columns but is the only field in its row, leaving empty space. Consider adjusting the layout for better space utilization.
-<div className="grid grid-cols-2 gap-4"> +<div className="w-full"> <FormField control={form.control} name="gender" render={({ field }) => ( - <FormItem> + <FormItem className="w-1/2">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Users/UserForm.tsx(12 hunks)src/components/Users/UserSummary.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Users/UserSummary.tsx
🧰 Additional context used
📓 Learnings (1)
src/components/Users/UserForm.tsx (2)
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.
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#10027
File: src/components/Users/UserForm.tsx:34-34
Timestamp: 2025-01-17T11:34:37.003Z
Learning: In the care_fe codebase, the GENDERS constant is defined with a type assertion `as [(typeof GENDER_TYPES)[number]["id"]]` which makes it compatible with `z.enum()` in Zod schemas. This pattern is preferred over using string literals directly.
|
@Jacobjeevan 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
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
Localization
New Features
Improvements
User Experience