-
Notifications
You must be signed in to change notification settings - Fork 514
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
Replaced deprecated useMutation in questionnaire with tanstack's useMutation #9840
base: develop
Are you sure you want to change the base?
Replaced deprecated useMutation in questionnaire with tanstack's useMutation #9840
Conversation
WalkthroughThe pull request updates the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 requesting for a review. |
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 (3)
src/components/Questionnaire/QuestionnaireForm.tsx (3)
81-93
: Enhance mutation implementation with better error handling and type safetyWhile the migration to TanStack's
useMutation
is good, consider these improvements:
- Add type safety for the error object
- Provide more context in error messages
- Consider adding retry strategy for transient failures
const { mutate: submitBatch, isPending: isProcessing } = useMutation({ mutationFn: mutate(routes.batchRequest), + retry: (failureCount, error) => failureCount < 3 && isRetryableError(error), onSuccess: () => { toast.success("Questionnaire submitted successfully"); onSubmit?.(); }, - onError: (error: any) => { + onError: (error: Error & { results?: ValidationErrorResponse[] }) => { if (error.results) { handleSubmissionError(error.results as ValidationErrorResponse[]); } - toast.error("Failed to submit questionnaire"); + toast.error(`Failed to submit questionnaire: ${error.message || 'Unknown error occurred'}`); }, });
246-247
: Add error boundary around mutation executionThe mutation execution could benefit from try-catch error handling to gracefully handle synchronous errors.
- // Execute the mutation - submitBatch({ requests }); + try { + // Execute the mutation + submitBatch({ requests }); + } catch (error) { + console.error('Failed to execute mutation:', error); + toast.error('An unexpected error occurred while submitting the questionnaire'); + }
212-214
: Consider adding explicit type predicate for response filteringThe response filtering logic could be more explicit with a type predicate function.
+ const isNonStructuredResponse = (response: QuestionnaireResponse): boolean => !response.structured_type; + - const nonStructuredResponses = form.responses.filter( - (response) => !response.structured_type, - ); + const nonStructuredResponses = form.responses.filter(isNonStructuredResponse);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Questionnaire/QuestionnaireForm.tsx
(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
src/components/Questionnaire/QuestionnaireForm.tsx (2)
81-93
: Overall implementation looks good!The migration to TanStack's
useMutation
is a positive change that improves the code structure and maintainability. While there are some suggested improvements, the core implementation is solid.
1-1
: Consider removing the custom mutate utility importSince we're migrating to TanStack's
useMutation
, the custommutate
utility import might be redundant. Consider removing it if it's no longer needed elsewhere in the codebase.Let's verify if the custom mutate utility is still needed:
Also applies to: 16-16
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/Questionnaire/QuestionnaireForm.tsx (2)
82-95
: Improve error handling type safetyThe error handling could be more type-safe. Consider creating a custom type for the error response structure.
- onError: (error: HTTPError) => { - const errorData = error.cause; - if (errorData?.results) { + onError: (error: HTTPError) => { + type BatchErrorResponse = { + results?: ValidationErrorResponse[]; + }; + const errorData = error.cause as BatchErrorResponse; + if (errorData?.results) {
Line range hint
214-248
: Extract submission preparation logic for better readabilityConsider extracting the request preparation logic into a separate helper function to improve code organization.
+ const prepareSubmissionRequest = (form: QuestionnaireFormState): BatchRequest | null => { + const nonStructuredResponses = form.responses.filter( + (response) => !response.structured_type, + ); + + if (nonStructuredResponses.length === 0) return null; + + return { + url: `/api/v1/questionnaire/${form.questionnaire.slug}/submit/`, + method: "POST", + reference_id: form.questionnaire.id, + body: { + resource_id: encounterId, + encounter: encounterId, + patient: patientId, + results: nonStructuredResponses + .filter((response) => response.values.length > 0) + .map((response) => ({ + question_id: response.question_id, + values: response.values.map((value) => ({ + ...(value.value_code + ? { value_code: value.value_code } + : { value: String(value.value) }), + })), + note: response.note, + body_site: response.body_site, + method: response.method, + })), + }, + }; + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json
(1 hunks)src/components/Questionnaire/QuestionnaireForm.tsx
(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🔇 Additional comments (2)
src/components/Questionnaire/QuestionnaireForm.tsx (2)
Line range hint
413-422
: LGTM! Well-implemented loading stateThe loading spinner implementation is clean and follows accessibility best practices by maintaining button dimensions and providing visual feedback.
Line range hint
1-434
: Verify complete migration from deprecated data fetchingLet's ensure no deprecated data fetching utilities remain in the codebase.
✅ Verification successful
Migration to TanStack's useMutation is complete
All useMutation imports across the codebase are properly sourced from @tanstack/react-query, and no deprecated data fetching patterns were found. The changes in QuestionnaireForm.tsx are consistent with the rest of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of deprecated data fetching patterns # This helps ensure the migration to TanStack's useMutation is complete echo "Checking for deprecated mutation patterns..." rg -l "useMutation.*from\s+['\"](?!@tanstack)" echo "Checking for old mutation utility imports..." rg -l "import.*mutation.*from\s+['\"]@/Utils/request/mutation['\"]"Length of output: 587
Script:
#!/bin/bash # Check for useMutation imports echo "=== Checking useMutation imports ===" rg "import.*useMutation.*from" -g '!*.test.*' -g '!*.spec.*' # Check for old mutation utility imports and usage echo -e "\n=== Checking old mutation utilities ===" rg "from\s+['\"]@/Utils/request/mutation['\"]" rg "mutation\(" # Check for direct API calls without proper mutation handling echo -e "\n=== Checking direct API calls ===" rg "\.post\(|\.put\(|\.patch\(|\.delete\(" -g '*.tsx' -g '*.ts' -A 2Length of output: 5020
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.
now that we are no longer using the old custom useMutation
hook, let's delete that file too btw
Proposed Changes
part 1: useMutation sloved
(updating the
QuestionnaireForm.tsx
component to replace the custom mutation hook with the @tanstack/react-query library'suseMutation
)@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
@tanstack/react-query
.