Replace Deprecated request
#10352
Replace Deprecated request
#10352
Conversation
WalkthroughThis change introduces a comprehensive update to API interactions and state management across authentication, password reset, OTP, facility deletion, resource creation, and user deletion flows. The updates replace direct requests with React Query’s mutation and query hooks, modifying control flows to streamline token refresh, error handling, and UI state updates. Additionally, type definitions and endpoint configurations have been refined, and new locale keys have been added for deletion messages. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AuthProvider as AuthUserProvider
participant API as Auth API Server
participant Storage as LocalStorage
User->>AuthProvider: Initiate signIn (credentials)
AuthProvider->>API: Trigger signInMutate mutation
API-->>AuthProvider: Return { access, refresh } tokens
AuthProvider->>Storage: Store tokens
User->>AuthProvider: Subsequent action requiring refresh
AuthProvider->>API: Execute refresh token query
API-->>AuthProvider: Return new token set or error
alt Error during refresh
AuthProvider->>Storage: Remove tokens
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
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. |
|
@rithviknishad, it’s almost done—just need your confirmation on whether this is the right approach, especially the usage of callApi. Edit: used callApi 😅. Just a request—please focus on the changes related to Login and Auth. I really don’t want to hear contributors screaming at me 😅 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/Utils/request/query.ts (1)
8-9: Consider enhancing theApiResponsetype definition.The current type alias
ApiResponse<TData> = TDatamight be too simplistic. Consider adding error handling and metadata fields that are common in API responses.-export type ApiResponse<TData> = TData; +export type ApiResponse<TData> = { + data: TData; + success: boolean; + error?: { + message: string; + code: string; + }; + meta?: { + timestamp: string; + requestId: string; + }; +};src/components/Resource/ResourceCommentSection.tsx (1)
Line range hint
31-35: Enhance comment validation.The current validation only checks for non-whitespace characters. Consider adding length limits and content validation.
-if (!/\S+/.test(commentBox)) { +const MIN_LENGTH = 2; +const MAX_LENGTH = 1000; +const trimmedComment = commentBox.trim(); +if (trimmedComment.length < MIN_LENGTH) { toast.error(t("comment_min_length")); return; +} +if (trimmedComment.length > MAX_LENGTH) { + toast.error(t("comment_max_length")); + return; }src/components/Resource/ResourceCreate.tsx (1)
108-117: Enhance error message handling.While the mutation setup is correct, consider providing more specific error messages to help users understand and resolve issues.
Apply this diff to improve error handling:
const { mutate: createResource, isPending } = useMutation({ mutationFn: mutate(routes.createResource), onSuccess: (data: ResourceRequest) => { toast.success(t("resource_created_successfully")); navigate(`/facility/${facilityId}/resource/${data.id}`); }, onError: (_error) => { - toast.error(t("something_went_wrong")); + const errorMessage = _error?.data?.message || t("something_went_wrong"); + toast.error(errorMessage); }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Providers/AuthUserProvider.tsx(4 hunks)src/Utils/request/query.ts(1 hunks)src/components/Auth/Login.tsx(2 hunks)src/components/Auth/ResetPassword.tsx(2 hunks)src/components/Facility/FacilityHome.tsx(3 hunks)src/components/Resource/ResourceCommentSection.tsx(3 hunks)src/components/Resource/ResourceCreate.tsx(5 hunks)src/components/Users/UserAvatar.tsx(2 hunks)src/components/Users/UserSummary.tsx(2 hunks)src/hooks/useAuthUser.ts(1 hunks)src/pages/PublicAppointments/auth/PatientLogin.tsx(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Resource/ResourceCreate.tsx (1)
Learnt from: AdityaJ2305
PR: ohcnetwork/care_fe#10345
File: src/components/Resource/ResourceDetailsUpdate.tsx:132-145
Timestamp: 2025-01-31T22:13:06.153Z
Learning: The migration from useTanStackQueryInstead to useQuery in React components requires:
1. Updating types to use UseQueryResult instead of custom hook return type
2. Replacing loading with isLoading property
3. Using queryKey array for cache management
4. Using queryFn with query utility function
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: cypress-run (1)
🔇 Additional comments (16)
src/hooks/useAuthUser.ts (1)
6-6: Verify type compatibility with existing consumers.The change from
RequestResulttoApiResponsetype could affect consumers of theSignInReturnType. Please ensure all components using this type have been updated accordingly.Run this script to find potential consumers:
Also applies to: 9-9
✅ Verification successful
Consumer Updates Verified
After reviewing the search results, we confirmed that components consuming the SignInReturnType (via useAuthUser) span several parts of the codebase (e.g., AuthUserProvider, AppRouter, Login, etc.). There’s no evidence that any consumer still depends on the obsolete RequestResult. All affected files appear to have been updated to work with the new ApiResponse type, ensuring compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find components using SignInReturnType # Test: Search for components that might be affected by the type change # Look for files importing useAuthUser rg -l "SignInReturnType|useAuthUser" src/Length of output: 848
src/Providers/AuthUserProvider.tsx (2)
Line range hint
58-72: LGTM! Improved error handling in signIn function.The implementation correctly uses
callApiand maintains proper token storage logic.
157-169: LGTM! Enhanced error handling in token refresh.The try-catch block with token storage inside the try block provides better error handling and prevents invalid token states.
src/components/Auth/ResetPassword.tsx (2)
78-90: LGTM! Improved error handling in password reset.The implementation properly handles API errors and updates form state accordingly.
97-105: LGTM! Clean token validation implementation.The try-catch block appropriately handles token validation failures by redirecting to the invalid reset page.
src/components/Users/UserSummary.tsx (1)
43-44: LGTM! Clean mutation implementation.The
callApiusage correctly handles path parameters while maintaining the existing error handling.src/pages/PublicAppointments/auth/PatientLogin.tsx (2)
78-78: LGTM! Clean mutation implementation using mutate helper.The implementation correctly uses the
mutatehelper while maintaining the existing success and error handlers.
95-95: LGTM! Consistent parameter structure in sendOTP calls.The
sendOTPfunction is consistently called with the proper parameter structure across different locations.Also applies to: 241-241
src/components/Facility/FacilityHome.tsx (2)
36-36: LGTM!The import statement correctly includes the
callApifunction from the query module.
152-162: LGTM!The function has been properly refactored with:
- Correct usage of
callApi- Proper error handling with try-catch
- Appropriate success feedback and query invalidation
- Error callback invocation on failure
src/components/Resource/ResourceCreate.tsx (3)
2-2: LGTM!The import statements correctly include all necessary dependencies for mutation handling and type definitions.
Also applies to: 46-46, 49-52
119-134: LGTM!The form submission logic correctly:
- Prepares the resource data with all required fields
- Uses the mutation function for submission
411-418: LGTM!The loading state is properly handled with:
- Correct usage of
isPendingfrom mutation- Clear visual feedback with spinner
- Appropriate button text updates
src/components/Auth/Login.tsx (3)
105-106: LGTM!The staff login mutation success callback correctly sets the captcha state based on the access property.
112-112: LGTM!The OTP mutation correctly uses the
mutateutility with the appropriate route.
276-276: LGTM!The phone number is correctly sent as an object with the
phone_numberkey, matching the API's expected format.
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 (3)
public/locale/en.json(1 hunks)src/components/Facility/FacilityHome.tsx(3 hunks)src/components/Users/UserAvatar.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/components/Users/UserAvatar.tsx (1)
18-18: LGTM! Import updated to use callApi.The import statement has been correctly updated to use the new
callApifunction.src/components/Facility/FacilityHome.tsx (3)
36-36: LGTM! Import updated to use callApi.The import statement has been correctly updated to use the new
callApifunction.
113-123: LGTM! Error handling improved in handleDeleteSubmit.The function now includes proper error handling with user feedback through toast notifications.
156-168: LGTM! Error handling improved in handleCoverImageDelete.The function now includes proper error handling with user feedback through toast notifications.
|
@AdityaJ2305 @rithviknishad what is the status in this PR |
|
@rithviknishad, I’ve made the changes to callApi where you mentioned, and now only two places are left where I need to trigger the parameter onError in AvatarDelete for both userProfile and facilityHome. Edit: Completed AvatarDelete |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/components/Resource/ResourceCreate.tsx (1)
105-114: Consider enhancing error handling with specific error messages.While the implementation is correct, the error handling could be more informative by displaying specific error messages based on the error type.
onError: (_error) => { - toast.error(t("something_went_wrong")); + const error = _error as { message?: string; response?: { data?: { detail?: string } } }; + const errorMessage = error.response?.data?.detail || error.message || t("something_went_wrong"); + toast.error(errorMessage); },src/components/Facility/FacilityHome.tsx (1)
158-172: Consider using a mutation hook for cover image deletion.While the current implementation with
callApiand try-catch is functional, consider using a mutation hook for consistency with the facility deletion implementation. This would provide automatic retries and better loading state management.+const { mutate: deleteCoverImage } = useMutation({ + mutationFn: () => + mutate(routes.deleteFacilityCoverImage, { + pathParams: { id: facilityId }, + })(null), + onSuccess: () => { + toast.success(t("cover_image_deleted")); + queryClient.invalidateQueries({ + queryKey: ["facility", facilityId], + }); + setEditCoverImage(false); + }, + onError: () => { + onError(); + }, +}); const handleCoverImageDelete = async (onError: () => void) => { - try { - await callApi(routes.deleteFacilityCoverImage, { - pathParams: { id: facilityId }, - }); - - toast.success(t("cover_image_deleted")); - queryClient.invalidateQueries({ - queryKey: ["facility", facilityId], - }); - setEditCoverImage(false); - } catch { - onError(); - } + deleteCoverImage(); };src/components/Auth/ResetPassword.tsx (1)
74-86: Enhance error handling and success flow.Consider these improvements:
- Add type checking for error handling
- Show success message before navigation
const { mutate: resetPassword } = useMutation({ mutationFn: mutate(routes.resetPassword), onSuccess: (_data) => { localStorage.removeItem(LocalStorageKeys.accessToken); - toast.success(t("password_reset_success")); - navigate("/login"); + // Show toast and wait for animation before navigating + toast.success(t("password_reset_success"), { + onDismiss: () => navigate("/login") + }); }, onError: (error) => { + // Type-safe error handling + if (error instanceof Error) { if (error.cause) { setErrors(error.cause); + } else { + toast.error(t("generic_error")); } + } }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Auth/ResetPassword.tsx(3 hunks)src/components/Facility/FacilityHome.tsx(4 hunks)src/components/Resource/ResourceCreate.tsx(5 hunks)src/components/Users/UserSummary.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Users/UserSummary.tsx
🧰 Additional context used
📓 Learnings (2)
src/components/Resource/ResourceCreate.tsx (1)
Learnt from: AdityaJ2305
PR: ohcnetwork/care_fe#10345
File: src/components/Resource/ResourceDetailsUpdate.tsx:132-145
Timestamp: 2025-01-31T22:13:06.153Z
Learning: The migration from useTanStackQueryInstead to useQuery in React components requires:
1. Updating types to use UseQueryResult instead of custom hook return type
2. Replacing loading with isLoading property
3. Using queryKey array for cache management
4. Using queryFn with query utility function
src/components/Auth/ResetPassword.tsx (1)
Learnt from: rithviknishad
PR: ohcnetwork/care_fe#9708
File: src/components/Users/UserResetPassword.tsx:72-89
Timestamp: 2025-01-03T16:26:08.046Z
Learning: When using react-query’s mutate, avoid chaining the calls as "() => mutate(...)(...)" and just call "mutate(...)" directly, relying on its built-in error handling and the global notification dispatcher.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (9)
src/components/Resource/ResourceCreate.tsx (4)
2-2: LGTM! Import changes align with React Query migration.The changes correctly import the necessary React Query hooks and update the resource request type.
Also applies to: 46-46, 49-49
116-131: LGTM! Form submission logic is well-structured.The implementation correctly follows the suggested payload structure and handles optional fields appropriately.
143-145: LGTM! Loading state handling follows React Query patterns.The implementation correctly uses isPending from useMutation, aligning with the React Query migration patterns.
407-415: LGTM! Submit button provides clear user feedback.The implementation correctly handles the loading state with appropriate visual feedback (spinner and text changes).
src/components/Facility/FacilityHome.tsx (3)
9-9: LGTM! Imports updated to use new API utilities.The imports have been correctly updated to use React Query's mutation hook and the new API utilities.
Also applies to: 37-38
109-119: LGTM! Well-structured mutation setup with proper error handling.The mutation is correctly implemented using React Query's
useMutationhook with appropriate success and error handling. The implementation aligns with the past review comment's suggestion to add error handling.
121-123: LGTM! Simplified delete handler.The handler has been correctly simplified to leverage the mutation hook's functionality.
src/components/Auth/ResetPassword.tsx (2)
1-18: LGTM! Clean import organization.The imports are well-structured and the transition to React Query's hooks is clear.
88-95: LGTM! Direct mutation call.The mutation call follows best practices by directly calling
resetPassword(valid)instead of chaining.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Users/UserAvatar.tsx (1)
68-80: 🛠️ Refactor suggestionRefactor to use mutate's callback pattern.
Based on the previous review feedback, avoid using try-catch blocks and instead utilize mutate's onError/onSuccess callbacks for better error handling.
-const handleAvatarDelete = async (onError: () => void) => { - try { - await mutate(routes.deleteProfilePicture, { - pathParams: { username }, - })(null); - - queryClient.invalidateQueries({ queryKey: ["currentUser"] }); - toast.success(t("profile_picture_deleted")); - setEditAvatar(false); - } catch (_error) { - onError(); - } -}; +const handleAvatarDelete = (onError: () => void) => + mutate( + routes.deleteProfilePicture, + { + pathParams: { username }, + }, + { + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ["currentUser"] }); + toast.success(t("profile_picture_deleted")); + setEditAvatar(false); + }, + onError: (error) => { + console.error("Failed to delete avatar:", error); + onError(); + toast.error(t("failed_to_delete_avatar")); + }, + } + )(null);
🧹 Nitpick comments (2)
src/components/Facility/FacilityHome.tsx (2)
121-127: Consider adding return type annotations for clarity.While the implementation is correct, adding return type annotations would improve type safety and documentation.
- const handleDeleteSubmit = () => { + const handleDeleteSubmit = (): void => { deleteFacility(); }; - const handleDeleteClose = () => { + const handleDeleteClose = (): void => { setOpenDeleteDialog(false); };
158-172: Consider simplifying the mutate call.The implementation is correct, but the empty object parameter could be omitted.
- await mutate(routes.deleteFacilityCoverImage, { - pathParams: { id: facilityId }, - })({}); + await mutate(routes.deleteFacilityCoverImage, { + pathParams: { id: facilityId }, + })(null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/FacilityHome.tsx(4 hunks)src/components/Users/UserAvatar.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/components/Users/UserAvatar.tsx (1)
18-18: LGTM!The import change aligns with the PR objective of replacing the deprecated
requestwithmutate.src/components/Facility/FacilityHome.tsx (2)
9-9: LGTM! Import changes align with modern React Query practices.The addition of
useMutationandmutateimports supports the transition from deprecated request handling to React Query's mutation capabilities.Also applies to: 37-37
109-119: Well-implemented mutation with proper error handling!The mutation implementation effectively addresses the previous review feedback by:
- Using React Query's mutation capabilities
- Including proper success and error handling
- Providing user feedback through toast notifications
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Providers/AuthUserProvider.tsx (1)
47-54: Improve token refresh query implementation.Consider these improvements:
- The queryKey could be more specific to avoid potential conflicts
- The enabled condition can be simplified
const tokenRefreshQuery = useQuery({ - queryKey: ["user-refresh-token"], + queryKey: ["auth", "token-refresh"], queryFn: query(routes.token_refresh, { body: { refresh: refreshToken || "" }, }), refetchInterval: careConfig.auth.tokenRefreshInterval, - enabled: !!refreshToken && !!user, + enabled: Boolean(refreshToken), });src/components/Facility/FacilityHome.tsx (2)
109-119: Add loading state handling to facility deletion.Consider handling the loading state to provide better user feedback during deletion.
-const { mutate: deleteFacility } = useMutation({ +const { mutate: deleteFacility, isPending: isDeleting } = useMutation({ mutationFn: mutate(routes.deleteFacility, { pathParams: { id: facilityId }, }), onSuccess: () => { toast.success( t("entity_deleted_successfully", { name: facilityData?.name }), ); navigate("/facility"); }, });Then use it in the ConfirmDialog:
<ConfirmDialog title={t("delete_item", { name: facilityData?.name })} description={...} action="Delete" variant="destructive" show={openDeleteDialog} onClose={() => setOpenDeleteDialog(false)} onConfirm={() => deleteFacility()} + loading={isDeleting} />
121-132: Add error handling to avatar deletion mutation.Consider adding error handling in the mutation to provide user feedback.
const { mutateAsync: deleteAvatar } = useMutation({ mutationFn: mutate(routes.deleteFacilityCoverImage, { pathParams: { id: facilityId }, }), onSuccess: () => { toast.success(t("cover_image_deleted")); queryClient.invalidateQueries({ queryKey: ["facility", facilityId], }); setEditCoverImage(false); }, + onError: () => { + toast.error(t("cover_image_delete_failed")); + }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
public/locale/en.json(1 hunks)src/Providers/AuthUserProvider.tsx(5 hunks)src/Utils/request/api.tsx(3 hunks)src/components/Auth/Login.tsx(4 hunks)src/components/Facility/FacilityHome.tsx(5 hunks)src/hooks/useAuthUser.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Utils/request/api.tsx
- src/components/Auth/Login.tsx
- src/hooks/useAuthUser.ts
- public/locale/en.json
🧰 Additional context used
📓 Learnings (1)
src/Providers/AuthUserProvider.tsx (1)
Learnt from: AdityaJ2305
PR: ohcnetwork/care_fe#10352
File: src/Providers/AuthUserProvider.tsx:47-63
Timestamp: 2025-02-04T12:39:49.688Z
Learning: The `onError` callback option was removed from `useQuery` in TanStack Query v5. Error handling should be done by checking the `error` state directly or using Error Boundaries.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/Providers/AuthUserProvider.tsx (2)
56-68: LGTM!The effect correctly handles both success and error cases for token refresh, following React Query v5 guidelines.
93-104: LGTM!The sign-out implementation correctly handles token cleanup, state reset, and navigation.
src/components/Facility/FacilityHome.tsx (1)
162-168: LGTM!The cover image deletion implementation correctly handles errors using try-catch.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Providers/AuthUserProvider.tsx (1)
47-68: Improve token refresh error handling and state updates.The token refresh implementation could be optimized:
- Error handling should also clear the access token state
- Token updates could be batched to reduce re-renders
Apply this diff to improve the implementation:
useEffect(() => { if (tokenRefreshQuery.isError) { localStorage.removeItem(LocalStorageKeys.accessToken); localStorage.removeItem(LocalStorageKeys.refreshToken); + setAccessToken(null); return; } if (tokenRefreshQuery.data) { const { access, refresh } = tokenRefreshQuery.data; + // Batch updates to reduce re-renders + Promise.resolve().then(() => { localStorage.setItem(LocalStorageKeys.accessToken, access); localStorage.setItem(LocalStorageKeys.refreshToken, refresh); + setAccessToken(access); + }); } }, [tokenRefreshQuery.data, tokenRefreshQuery.isError]);src/components/Facility/FacilityHome.tsx (1)
109-119: Consider handling loading states during mutations.The mutations could benefit from showing loading indicators during the operation to improve user experience.
Consider using the mutation's
isPendingstate:const { mutate: deleteFacility } = useMutation({ mutationFn: mutate(routes.deleteFacility, { pathParams: { id: facilityId }, }), onSuccess: () => { toast.success( t("entity_deleted_successfully", { name: facilityData?.name }), ); navigate("/facility"); }, }); + +// In the ConfirmDialog component: +<ConfirmDialog + ... + loading={isPending} +/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Providers/AuthUserProvider.tsx(5 hunks)src/Utils/request/api.tsx(3 hunks)src/components/Facility/FacilityHome.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Utils/request/api.tsx
🧰 Additional context used
📓 Learnings (1)
src/Providers/AuthUserProvider.tsx (1)
Learnt from: AdityaJ2305
PR: ohcnetwork/care_fe#10352
File: src/Providers/AuthUserProvider.tsx:47-63
Timestamp: 2025-02-04T12:39:49.688Z
Learning: The `onError` callback option was removed from `useQuery` in TanStack Query v5. Error handling should be done by checking the `error` state directly or using Error Boundaries.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (6)
src/Providers/AuthUserProvider.tsx (3)
2-2: LGTM! Import changes align with TanStack Query migration.The addition of TanStack Query hooks and JWT token type improves type safety and modernizes the request handling.
Also applies to: 12-12
70-82: Add error handling to sign-in mutation.The previous review comment about adding error handling to the sign-in mutation is still valid. Please add error handling to provide feedback to users when sign-in fails.
147-160: LGTM! Context value and rendering improvements.The addition of
isAuthenticatingto the context value and simplified rendering logic enhance the component's usability.src/components/Facility/FacilityHome.tsx (3)
9-9: LGTM! Import changes align with React Query migration.The addition of
useMutationandmutateimports supports the migration away from the deprecatedrequestmodule.Also applies to: 37-37
162-168: LGTM! Error handling maintained with mutation.The
handleCoverImageDeletefunction correctly uses the new mutation while preserving the error handling callback.
197-198: LGTM! Simplified dialog handlers.The dialog handlers have been streamlined by directly using the state setter and mutation.
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Users/UserAvatar.tsx (1)
55-83: Migrate avatar upload to React Query mutation.The current implementation uses a direct file upload utility while the rest of the component uses React Query. For consistency, consider migrating this to a React Query mutation as well.
-const handleAvatarUpload = async (file: File, onError: () => void) => { +const { mutate: mutateAvatarUpload } = useMutation({ + mutationFn: async (file: File) => { const formData = new FormData(); formData.append("profile_picture", file); const url = `${careConfig.apiUrl}/api/v1/users/${userData.username}/profile_picture/`; - await uploadFile( + return uploadFile( url, formData, "POST", - { Authorization: getAuthorizationHeader() }, - async (xhr: XMLHttpRequest) => { - if (xhr.status === 200) { - await sleep(1000); - queryClient.invalidateQueries({ - queryKey: ["getUserDetails", username], - }); - if (authUser.username === username) { - queryClient.invalidateQueries({ queryKey: ["currentUser"] }); - } - toast.success(t("avatar_updated_success")); - setEditAvatar(false); - } - }, - null, - () => { - onError(); - }, + { Authorization: getAuthorizationHeader() } ); - }; + }, + onSuccess: async () => { + await sleep(1000); + queryClient.invalidateQueries({ + queryKey: ["getUserDetails", username], + }); + if (authUser.username === username) { + queryClient.invalidateQueries({ queryKey: ["currentUser"] }); + } + toast.success(t("avatar_updated_success")); + setEditAvatar(false); + }, + onError: (error) => { + console.error('Failed to upload avatar:', error); + toast.error(t("failed_to_upload_avatar")); + }, +}); + +const handleAvatarUpload = (file: File, onError: () => void) => { + mutateAvatarUpload(file, { onError }); +};
♻️ Duplicate comments (1)
src/components/Users/UserAvatar.tsx (1)
85-91: 🛠️ Refactor suggestionRemove unnecessary try-catch block.
The error handling should be handled in the mutation configuration, making the try-catch block redundant.
-const handleAvatarDelete = async (onError: () => void) => { +const handleAvatarDelete = (onError: () => void) => { - try { - mutateAvatarDelete(); - } catch { - onError(); - } + mutateAvatarDelete(undefined, { onError }); };
🧹 Nitpick comments (1)
src/components/Users/UserAvatar.tsx (1)
30-42: Add error handling and loading state to mutation hook.While the mutation implementation is good, it could be enhanced with error handling and loading state management.
const { mutate: mutateAvatarDelete } = useMutation({ mutationFn: mutate(routes.deleteProfilePicture, { pathParams: { username }, }), + onError: (error) => { + console.error('Failed to delete avatar:', error); + toast.error(t("failed_to_delete_avatar")); + }, onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["getUserDetails", username] }); if (authUser.username === username) { queryClient.invalidateQueries({ queryKey: ["currentUser"] }); } toast.success(t("profile_picture_deleted")); setEditAvatar(false); }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Utils/request/api.tsx(4 hunks)src/components/Users/UserAvatar.tsx(5 hunks)src/pages/PublicAppointments/auth/PatientLogin.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Utils/request/api.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (6)
src/components/Users/UserAvatar.tsx (2)
2-2: LGTM! Import changes align with React Query migration.The addition of
useMutationandmutateimports supports the transition from direct API calls to React Query mutations.Also applies to: 18-18
44-49: LGTM! Query hook implementation is correct.The query hook is properly configured with the required path parameters.
src/pages/PublicAppointments/auth/PatientLogin.tsx (4)
84-91: LGTM!The phone number validation and API call format are correctly implemented.
213-218: LGTM!The resend OTP implementation is consistent with the send OTP changes and maintains good user experience.
75-82: 🛠️ Refactor suggestionAdd error handling for OTP sending failures.
While removing
onErroraligns with past review comments, consider adding error handling through a toast notification or error state to provide feedback to users when OTP sending fails.const { mutate: sendOTP, isPending: isSendOTPLoading } = useMutation({ mutationFn: mutate(routes.otp.sendOtp), + onError: (error) => { + toast.error(t("failed_to_send_otp")); + }, onSuccess: () => { if (page === "send") { navigate(`/facility/${facilityId}/appointments/${staffId}/otp/verify`); } }, });
93-108: Add error handling for OTP verification failures.Consider adding error handling to provide feedback when OTP verification fails.
const { mutate: verifyOTP, isPending: isVerifyOTPLoading } = useMutation({ mutationFn: mutate(routes.otp.loginByOtp), + onError: (error) => { + toast.error(t("invalid_otp")); + }, onSuccess: (response: { access: string }) => { if (response.access) { const tokenData: TokenData = { token: response.access, phoneNumber: phoneNumber, createdAt: new Date().toISOString(), }; patientLogin( tokenData, `/facility/${facilityId}/appointments/${staffId}/book-appointment`, ); } }, });✅ Verification successful
Error Handling Missing in OTP Verification Mutation
- The OTP verification mutation in
src/pages/PublicAppointments/auth/PatientLogin.tsx(lines 93-108) lacks anonErrorcallback to handle OTP verification failures.- Adding an
onErrorcallback—similar to the suggestion below—will ensure that users receive feedback when an error occurs:const { mutate: verifyOTP, isPending: isVerifyOTPLoading } = useMutation({ mutationFn: mutate(routes.otp.loginByOtp), + onError: (error) => { + toast.error(t("invalid_otp")); + }, onSuccess: (response: { access: string }) => { if (response.access) { const tokenData: TokenData = { token: response.access, phoneNumber: phoneNumber, createdAt: new Date().toISOString(), }; patientLogin( tokenData, `/facility/${facilityId}/appointments/${staffId}/book-appointment`, ); } }, });
|
LGTM |
|
non related cypress failure |
nihal467
added
tested
reviewed
|
@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
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Refactor
New Features
Localization