Adds useMutation compatible utility function and other abstractions
#9463
Conversation
WalkthroughThis pull request introduces significant changes to the API request and error handling infrastructure, focusing on the integration of TanStack Query. Key modifications include the renaming of functions, updates to error handling mechanisms, and the introduction of new utilities for API interactions. The changes centralize error management, enhance type safety, and provide more flexible options for making API calls, impacting multiple files across the project. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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 (
|
Deploying care-fe with
|
| Latest commit: |
62b35c5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e91fff6e.care-fe.pages.dev |
| Branch Preview URL: | https://rithviknishad-feat-mutation.care-fe.pages.dev |
✅ 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.
Actionable comments posted: 1
🧹 Nitpick comments (9)
src/Utils/request/mutate.ts (1)
19-26: Consider adding error handling example in documentation.The implementation looks good with proper type safety. However, consider enhancing the documentation to include an example of error handling with
onErrorcallback, as mutations often need to handle failure cases.Example addition to documentation:
/** * Example with error handling: * ```tsx * const { mutate } = useMutation({ * mutationFn: mutate(MedicineRoutes.createPrescription), * onError: (error) => { * if (error instanceof HTTPError) { * // Handle specific API errors * } * } * }); * ``` */src/Utils/request/errorHandler.ts (2)
Line range hint
37-44: Consider adding refresh token handling.The session expiration check is good, but consider adding refresh token logic before redirecting to session-expired page.
This would improve user experience by attempting to refresh the session before forcing a re-login.
52-54: Consider expanding bad request handling.The current implementation only checks status codes. Consider adding more granular error type checking for better error messages.
-function isBadRequest(error: HTTPError) { - return error.status === 400 || error.status === 406; +function isBadRequest(error: HTTPError) { + return ( + error.status === 400 || + error.status === 406 || + error.cause?.type === 'validation_error' + ); +}src/Utils/request/types.ts (1)
47-70: Consider adding error codes for better error handlingThe
HTTPErrorclass provides a good foundation for error handling. Consider enhancing it with error codes for more specific error handling scenarios.type HTTPErrorCause = Record<string, unknown> | undefined; +type HTTPErrorCode = 'NETWORK_ERROR' | 'VALIDATION_ERROR' | 'AUTH_ERROR' | 'SERVER_ERROR'; export class HTTPError extends Error { status: number; silent: boolean; cause?: HTTPErrorCause; + code: HTTPErrorCode; constructor({ message, status, silent, cause, + code, }: { message: string; status: number; silent: boolean; cause?: Record<string, unknown>; + code: HTTPErrorCode; }) { super(message, { cause }); this.status = status; this.silent = silent; this.cause = cause; + this.code = code; } }src/Utils/request/utils.ts (1)
53-62: Consider adding CSRF protection headersFor better security, consider adding CSRF protection headers when making state-changing requests.
export function makeHeaders(noAuth: boolean, additionalHeaders?: HeadersInit) { const headers = new Headers(additionalHeaders); headers.set("Content-Type", "application/json"); headers.append("Accept", "application/json"); + // Add CSRF token header for state-changing requests + const csrfToken = document.querySelector('meta[name="csrf-token"]')?.getAttribute('content'); + if (csrfToken) { + headers.append("X-CSRF-Token", csrfToken); + } const jwtAccessToken = localStorage.getItem(LocalStorageKeys.accessToken); if (jwtAccessToken && !noAuth) { headers.append("Authorization", `Bearer ${jwtAccessToken}`); }src/App.tsx (1)
35-37: Consider configuring retry options for mutationsWhile the mutation cache is properly configured, consider adding retry options specific to mutations, especially for handling network issues.
mutationCache: new MutationCache({ onError: handleHttpError, + defaultOptions: { + retry: (failureCount, error) => { + if (error instanceof HTTPError && error.status >= 400 && error.status < 500) { + return false; // Don't retry client errors + } + return failureCount < 3; // Retry server errors up to 3 times + }, + }, }),src/Utils/request/README.md (3)
70-75: LGTM! Consider adding type constraints example.The interface changes look good and properly support both queries and mutations. The documentation is clear and includes good examples.
Consider adding an example showing type constraints for the
bodyproperty:interface CreateUserBody { name: string; email: string; } // Example usage with type constraint const { mutate } = useMutation({ mutationFn: mutate<CreateUserBody>(routes.users.create) });
105-179: Consider enhancing mutation examples with error handling and optimistic updates.The mutation documentation is comprehensive but could benefit from additional examples showing:
- Error handling with
onErrorcallback- Optimistic updates using
onMutate/onError/onSettled- Cache invalidation with
queryClient.invalidateQueriesConsider adding this example:
function UpdatePatient({ patientId }: { patientId: string }) { const queryClient = useQueryClient(); const { mutate: updatePatient } = useMutation({ mutationFn: mutate(PatientRoutes.update, { pathParams: { id: patientId } }), onMutate: async (newData) => { // Optimistic update await queryClient.cancelQueries({ queryKey: ['patient', patientId] }); const previousData = queryClient.getQueryData(['patient', patientId]); queryClient.setQueryData(['patient', patientId], newData); return { previousData }; }, onError: (err, newData, context) => { // Roll back on error queryClient.setQueryData( ['patient', patientId], context?.previousData ); toast.error("Failed to update patient"); }, onSuccess: () => { // Invalidate and refetch queryClient.invalidateQueries({ queryKey: ['patient', patientId] }); toast.success("Patient updated successfully"); } }); return <PatientForm onSubmit={updatePatient} />; }
Line range hint
180-500: Consider adding documentation for advanced TanStack Query features.The migration guide is comprehensive, but could benefit from documentation about:
- Caching strategies and
staleTimeconfiguration- Prefetching data with
prefetchQuery- Infinite queries for pagination
- Parallel and dependent queries
Consider adding this section:
### Advanced Features #### Caching Configuration ```typescript useQuery({ queryKey: ['patient', id], queryFn: query(routes.patient.get), staleTime: 5 * 60 * 1000, // Data remains fresh for 5 minutes cacheTime: 30 * 60 * 1000 // Keep inactive data for 30 minutes });Prefetching Data
// In your router/navigation logic await queryClient.prefetchQuery({ queryKey: ['patient', id], queryFn: query(routes.patient.get) });Infinite Queries
const { data, fetchNextPage } = useInfiniteQuery({ queryKey: ['patients'], queryFn: query(routes.patient.list), getNextPageParam: (lastPage) => lastPage.next });</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 96bc802ec57505b4e53e46f4f58514998bdb066e and e3efb48b0a26fafea2e0b786ab2b3ac75f650471. </details> <details> <summary>📒 Files selected for processing (8)</summary> * `src/App.tsx` (3 hunks) * `src/Utils/request/README.md` (2 hunks) * `src/Utils/request/errorHandler.ts` (3 hunks) * `src/Utils/request/mutate.ts` (1 hunks) * `src/Utils/request/query.ts` (3 hunks) * `src/Utils/request/queryError.ts` (0 hunks) * `src/Utils/request/types.ts` (1 hunks) * `src/Utils/request/utils.ts` (1 hunks) </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * src/Utils/request/queryError.ts </details> <details> <summary>🔇 Additional comments (7)</summary> <details> <summary>src/Utils/request/mutate.ts (1)</summary> `4-18`: **LGTM! Well-documented with clear example.** The documentation effectively demonstrates the integration with TanStack Query's `useMutation` hook and includes a practical example with success handling. </details> <details> <summary>src/Utils/request/errorHandler.ts (1)</summary> Line range hint `6-35`: **Verify error handling in mutation scenarios.** The error handling implementation looks solid, but let's verify it works consistently with mutations. <details> <summary>✅ Verification successful</summary> The previous search might have been too specific. Let's broaden our search to understand how mutations and error handling are implemented across the codebase. This will help us verify if the error handler is being used consistently. --- **Error handling implementation is properly integrated with mutations** Based on the search results, the error handling implementation is consistent and properly integrated: 1. The `handleHttpError` function is globally configured in `App.tsx` for both queries and mutations through the React Query's `QueryCache` and `MutationCache`. 2. The codebase uses TanStack Query's `useMutation` with a custom `mutate` wrapper that integrates with the global error handling. 3. The documentation in `README.md` shows proper usage patterns, including the ability to suppress error notifications when needed using the `silent` option, which aligns with the error handler's implementation. 4. The deprecated `useDeprecatedMutation` is being phased out in favor of TanStack Query's `useMutation`, ensuring consistent error handling across the application. The error handling implementation is working as intended for mutations, with proper global configuration and consistent usage patterns throughout the codebase. </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Search for mutation error handling patterns rg -l 'useMutation.*onError' | while read -r file; do echo "=== $file ===" rg -A 5 'useMutation.*onError' "$file" doneLength of output: 45
Script:
#!/bin/bash # Let's try a broader search for mutation patterns and error handling rg -A 5 'useMutation' # Also search for direct usage of handleHttpError rg -A 3 'handleHttpError' # And look for other error handling patterns in mutation context rg -A 5 'mutation.*catch'Length of output: 4310
src/Utils/request/query.ts (2)
46-68: LGTM! Well-documented query function.
The implementation and documentation are excellent, with clear examples and proper signal handling for request cancellation.
Line range hint 7-42: Verify error propagation in network failures.
The implementation looks good with proper error handling, but let's verify error propagation in network failures.
Additionally, consider adding retry logic for transient network failures.
// Add retry logic with exponential backoff
const MAX_RETRIES = 3;
for (let attempt = 0; attempt < MAX_RETRIES; attempt++) {
try {
res = await fetch(url, fetchOptions);
break;
} catch (error) {
if (attempt === MAX_RETRIES - 1) throw new Error("Network Error");
await new Promise(r => setTimeout(r, Math.pow(2, attempt) * 1000));
}
}✅ Verification successful
Let me search for error handling patterns in the codebase to verify the implementation.
Let me try with a simpler regex pattern to analyze the error handling implementation.
Error handling implementation is adequate for network failures
The current implementation properly handles network failures by catching fetch errors and throwing a "Network Error". While adding retry logic could improve resilience, the current error propagation is sufficient for basic error handling needs:
- Catches network-level failures from fetch
- Throws a clear "Network Error" message
- Allows errors to properly bubble up to calling code
The suggestion for retry logic remains valid as a future enhancement, but it's not a critical requirement for the current implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for network error handling patterns
rg -A 3 'catch.*Network Error'
Length of output: 32
Script:
#!/bin/bash
# Search for network error handling patterns with broader context
rg -B 2 -A 5 'catch\s*{.*?' src/Utils/request/query.ts
# Search for error handling patterns in tests
fd "query.test" --exec rg -A 5 'catch'
Length of output: 197
Script:
#!/bin/bash
# Search for network error handling patterns with fixed regex
rg -B 2 -A 5 'catch' src/Utils/request/query.ts
# Search for error related tests
fd "query.test" -t f -x cat {}
Length of output: 236
src/Utils/request/types.ts (2)
38-45: LGTM: Well-structured API call options interface
The APICallOptions interface provides a comprehensive and type-safe way to configure API calls, properly handling path parameters, query parameters, body data, and HTTP headers.
72-75: LGTM: Proper type augmentation for React Query
The module augmentation correctly extends React Query's type system to use the custom HTTPError type.
src/App.tsx (1)
Line range hint 24-34: LGTM: Well-configured query client defaults
The query client configuration provides good defaults for caching and refetching behavior.
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
rithviknishad/feat/mutation-util
|
| Run status |
|
| Run duration | 05m 27s |
| Commit |
|
| Committer | Rithvik Nishad |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
139
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Users/UserProfile.tsx (1)
Line range hint
200-300: Improve validation logic for doctor experienceThe current validation for
doctor_experience_commenced_onhas a potential issue where the logic could allow negative values due to the OR condition placement.Consider updating the validation to be more precise:
- } else if ( - (states.form.user_type === "Doctor" && - Number(states.form.doctor_experience_commenced_on) >= 100) || - Number(states.form.doctor_experience_commenced_on) < 0 - ) { + } else if ( + states.form.user_type === "Doctor" && + (Number(states.form.doctor_experience_commenced_on) >= 100 || + Number(states.form.doctor_experience_commenced_on) < 0) + ) {
🧹 Nitpick comments (7)
.cursorrules (1)
34-34: Consider adding migration guidelines.Since this represents a significant change in data fetching approach, consider adding migration guidelines to help developers transition existing code from custom hooks to TanStack Query.
Suggested additions:
- Care uses TanStack Query for data fetching from the API along with query and mutate utilities for the queryFn and mutationFn. (Docs @ /Utils/request/README.md) +- For migrating existing API calls: + - Replace useQuery hook with TanStack Query's useQuery + - Use query utility for queryFn implementation + - Use mutate utility for mutations + - Refer to migration examples in the documentationsrc/Utils/request/utils.ts (2)
53-54: Consider implementing a more secure token management strategyWhile centralizing token access is a good practice, directly accessing localStorage for sensitive data like JWT tokens has security implications:
- Vulnerable to XSS attacks
- Makes it difficult to implement token rotation
- Couples token storage implementation to localStorage
Consider implementing a secure token management service that:
- Uses HttpOnly cookies for token storage
- Provides token rotation capabilities
- Abstracts the storage mechanism
- Implements CSRF protection
Example implementation:
// src/services/auth/tokenService.ts export class TokenService { private static instance: TokenService; private constructor() {} static getInstance(): TokenService { if (!this.instance) { this.instance = new TokenService(); } return this.instance; } getAccessToken(): string | null { // Implementation can be changed without affecting consumers return localStorage.getItem(LocalStorageKeys.accessToken); } setAccessToken(token: string): void { // Add token validation, rotation logic here } removeAccessToken(): void { // Cleanup logic } } export const tokenService = TokenService.getInstance();
57-68: LGTM! Consider adding type validation for additionalHeadersThe changes improve the function's flexibility and standards compliance. The use of the Headers API and proper set/append methods is good practice.
Consider adding runtime type validation for additionalHeaders:
export function makeHeaders(noAuth: boolean, additionalHeaders?: HeadersInit) { + if (additionalHeaders && typeof additionalHeaders !== 'object') { + throw new TypeError('additionalHeaders must be an object or Headers instance'); + } const headers = new Headers(additionalHeaders);src/components/Users/UserAvatar.tsx (1)
49-49: Consider implementing upload-specific authorizationWhile using the centralized token access is good, file uploads might benefit from additional security measures.
Consider implementing:
- Upload-specific temporary tokens
- Content-type validation
- File size limits
- Virus scanning integration
Example implementation for upload security:
async function getUploadToken(file: File): Promise<string> { const { token } = await request(routes.getUploadToken, { body: { contentType: file.type, size: file.size, purpose: 'avatar' } }); return token; }src/components/Facility/FacilityHome.tsx (1)
121-121: Consider extracting image upload logic into a reusable hookThe token handling is consistent with other components, but the upload logic is duplicated across components.
Consider creating a reusable hook:
function useImageUpload(options: { endpoint: string; onSuccess: () => void; fieldName?: string; }) { const handleUpload = async (file: File, onError: () => void) => { const formData = new FormData(); formData.append(options.fieldName || 'image', file); uploadFile( options.endpoint, formData, 'POST', { Authorization: `Bearer ${getJWTAccessToken()}` }, async (xhr: XMLHttpRequest) => { if (xhr.status === 200) { await sleep(1000); options.onSuccess(); } else { onError(); } }, null, onError ); }; return { handleUpload }; }src/components/Users/UserProfile.tsx (2)
Line range hint
150-200: Enhance password validation feedbackThe password validation could be improved in several ways:
- Error messages should be internationalized
- Password validation rules could be centralized
- Error feedback could be more immediate
Consider creating a centralized password validation utility:
// passwordUtils.ts export const PASSWORD_RULES = { minLength: { test: (p: string) => p.length >= 8, key: 'password_min_length' }, lowercase: { test: (p: string) => p !== p.toUpperCase(), key: 'password_lowercase' }, uppercase: { test: (p: string) => p !== p.toLowerCase(), key: 'password_uppercase' }, number: { test: (p: string) => /\d/.test(p), key: 'password_number' } };
Line range hint
1-1000: Consider component decomposition for better maintainabilityThis component is handling multiple responsibilities and could benefit from being split into smaller, more focused components:
- Profile View Component
- Profile Edit Form
- Password Change Form
- Language Settings
- Update Check Component
Consider breaking down this large component into smaller, more manageable pieces. This would improve:
- Code maintainability
- Testing capabilities
- Reusability
- Performance through better rendering optimization
Would you like me to help create a detailed plan for this refactoring?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.cursorrules(1 hunks)src/Utils/request/utils.ts(1 hunks)src/components/Facility/FacilityHome.tsx(3 hunks)src/components/Users/UserAvatar.tsx(2 hunks)src/components/Users/UserProfile.tsx(3 hunks)
🔇 Additional comments (2)
.cursorrules (1)
34-34: Verify documentation path and completeness.
The guideline change appropriately reflects the architectural shift to TanStack Query. However, let's verify that the referenced documentation exists and is complete.
✅ Verification successful
Documentation exists and is comprehensive
The referenced documentation at /Utils/request/README.md exists and is comprehensive, covering:
- TanStack Query integration and usage
queryandmutateutility functions with examples- Migration guidelines from legacy to modern patterns
- Backward compatibility through
useTanStackQueryInstead
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the documentation exists and contains necessary sections
# about query and mutate utilities
# Check if the README exists
if ! fd -t f "README.md" "src/Utils/request"; then
echo "README.md not found in src/Utils/request/"
exit 1
fi
# Check if README contains necessary sections
rg -i "query|mutate|tanstack" "src/Utils/request/README.md"
Length of output: 4010
src/components/Users/UserProfile.tsx (1)
511-511: LGTM: Improved security in token handling
The change to use getJWTAccessToken() instead of direct localStorage access is a good security practice as it centralizes token management.
|
👋 Hi, @rithviknishad, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@rithviknishad 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
queryfor useMutation #9447useMutationwithmutatedocs#93@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
mutatefunction for data mutations compatible with TanStack Query.Bug Fixes
Documentation
Refactor
QueryErrorclass to streamline error handling.