Structured Response View Cleanup #9797
Conversation
WalkthroughThe pull request focuses on refactoring the Changes
Sequence DiagramsequenceDiagram
participant Component as StructuredResponseView
participant QueryHook as useQuery
participant API as Backend API
Component->>QueryHook: Trigger query based on type
alt Symptoms
QueryHook->>API: Fetch symptom data
else Diagnoses
QueryHook->>API: Fetch diagnosis data
else Allergies
QueryHook->>API: Fetch allergy data
end
API-->>QueryHook: Return query results
QueryHook-->>Component: Provide data for rendering
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. |
Deploying care-fe with
|
| Latest commit: |
47b6dac
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b05d89c5.care-fe.pages.dev |
| Branch Preview URL: | https://structured-response-view-cle.care-fe.pages.dev |
![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 (4)
src/components/Facility/ConsultationDetails/StructuredResponseView.tsx (4)
57-58: Consider providing additional user-facing error details.
Whileconsole.errorhelps in dev environments, consider augmenting the UI error message to guide end users on possible remediation steps (e.g., retry).
65-65: Handle loading or empty states more gracefully.
IfsymptomQuery.dataisnullor undefined, an empty table or spinner could be displayed instead of rendering nothing.
69-71: Avoid rendering nothing if no data is found.
Similarly, for diagnoses, consider returning a placeholder or “No data” message to improve user experience.
75-75: Provide feedback on empty allergy records.
Similarly, show a placeholder or fallback ifallergyQuery.datais empty or undefined, instead of returning nothing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/ConsultationDetails/StructuredResponseView.tsx(2 hunks)src/types/notes/messages.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/types/notes/messages.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (4)
src/components/Facility/ConsultationDetails/StructuredResponseView.tsx (4)
13-13: Strengthened type safety is commendable.
Narrowing thetypeprop to"symptom" | "diagnosis" | "allergy_intolerance"helps prevent type-related bugs and provides clear intent.
25-26: Concise definition of common parameters.
DefiningbasePathParamsandqueryParamsas separate objects keeps the setup clean and maintains clarity for shared fields.
28-31: Good consolidation of parameter construction.
Centralizing parameter logic ingetParamsavoids duplication and keeps the code DRY.
51-55: Good approach to unify queries.
Mappingtypeto the respective query object for a single entry point considerably reduces conditional complexity.
| const diagnosisQuery = useQuery({ | ||
| queryKey: ["diagnosis"], | ||
| queryFn: query(diagnosisApi.retrieveDiagnosis, getParams("diagnosisId")), | ||
| enabled: type === "diagnosis" && !!id, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure each diagnosis fetch is uniquely identified.
Similar to the symptom query, updating the query key to include the id would prevent stale or incorrectly cached diagnosis data.
- queryKey: ["diagnosis"],
+ queryKey: ["diagnosis", id],📝 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.
| const diagnosisQuery = useQuery({ | |
| queryKey: ["diagnosis"], | |
| queryFn: query(diagnosisApi.retrieveDiagnosis, getParams("diagnosisId")), | |
| enabled: type === "diagnosis" && !!id, | |
| }); | |
| const diagnosisQuery = useQuery({ | |
| queryKey: ["diagnosis", id], | |
| queryFn: query(diagnosisApi.retrieveDiagnosis, getParams("diagnosisId")), | |
| enabled: type === "diagnosis" && !!id, | |
| }); |
| const allergyQuery = useQuery({ | ||
| queryKey: ["allergy_intolerance"], | ||
| queryFn: query(allergyApi.retrieveAllergy, getParams("allergyId")), | ||
| enabled: type === "allergy_intolerance" && !!id, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Distinguish allergy requests with the specific ID.
Including the id in the query key would help React Query accurately track and control fetches for different allergy records.
- queryKey: ["allergy_intolerance"],
+ queryKey: ["allergy_intolerance", id],📝 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.
| const allergyQuery = useQuery({ | |
| queryKey: ["allergy_intolerance"], | |
| queryFn: query(allergyApi.retrieveAllergy, getParams("allergyId")), | |
| enabled: type === "allergy_intolerance" && !!id, | |
| }); | |
| const allergyQuery = useQuery({ | |
| queryKey: ["allergy_intolerance", id], | |
| queryFn: query(allergyApi.retrieveAllergy, getParams("allergyId")), | |
| enabled: type === "allergy_intolerance" && !!id, | |
| }); |
| const symptomQuery = useQuery({ | ||
| queryKey: ["symptom"], | ||
| queryFn: query(symptomApi.retrieveSymptom, getParams("symptomId")), | ||
| enabled: type === "symptom" && !!id, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve query key to differentiate requests with unique IDs.
When fetching data keyed only by "symptom", React Query may not refetch properly if id changes. Consider including the id in the query key for correct caching and refetching behavior.
- queryKey: ["symptom"],
+ queryKey: ["symptom", id],📝 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.
| const symptomQuery = useQuery({ | |
| queryKey: ["symptom"], | |
| queryFn: query(symptomApi.retrieveSymptom, getParams("symptomId")), | |
| enabled: type === "symptom" && !!id, | |
| const symptomQuery = useQuery({ | |
| queryKey: ["symptom", id], | |
| queryFn: query(symptomApi.retrieveSymptom, getParams("symptomId")), | |
| enabled: type === "symptom" && !!id, |
CARE
|
||||||||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
structured-response-view-cleanup
|
| Run status |
|
| Run duration | 01m 48s |
| Commit |
|
| Committer | Amjith Titus |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
3
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/e2e/patient_spec/patient_search.cy.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Patient Search > search patient with phone number and verifies details |
Test Replay
Screenshots
|
|
|
👋 Hi, @amjithtitus09, 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/Facility/ConsultationDetails/StructuredResponseView.tsx (3)
31-36: Consider memoizing the parameter construction.While the parameter handling is well-organized, consider memoizing
basePathParamsandgetParamsto prevent unnecessary recreations on re-renders.- const basePathParams = { patientId }; - const queryParams = { encounter: encounterId }; - - const getParams = (idKey: string) => ({ + const basePathParams = useMemo(() => ({ patientId }), [patientId]); + const queryParams = useMemo(() => ({ encounter: encounterId }), [encounterId]); + + const getParams = useCallback((idKey: string) => ({ pathParams: { ...basePathParams, [idKey]: id }, queryParams, - }); + }), [basePathParams, queryParams, id]);
59-66: Enhance error handling implementation.While the error handling provides user feedback, consider these improvements:
- Use a proper logging service instead of
console.errorin production- Provide more user-friendly error messages
- Consider adding error boundaries
- if (currentQuery.error) { - console.error(`Error loading ${type}:`, currentQuery.error); - return <div>Error loading {type}</div>; + if (currentQuery.error) { + // Use your logging service here + logger.error(`Error loading ${type}:`, currentQuery.error); + return ( + <ErrorMessage + title={`Unable to load ${type} information`} + message="Please try again later or contact support if the problem persists." + /> + ); }
73-83: Consider making single-item array requirement more explicit.The table components receive arrays containing single items. Consider making this requirement more explicit in the component's type definitions or documentation.
// Add to type definitions type SingleItemArray<T> = [T]; // Document the requirement /** * @param symptoms - Single-item array containing the symptom to display */ interface SymptomTableProps { symptoms: SingleItemArray<Symptom>; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx(2 hunks)src/components/Facility/ConsultationDetails/StructuredResponseView.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (5)
src/components/Facility/ConsultationDetails/StructuredResponseView.tsx (3)
11-19: LGTM! Type definitions enhance type safety.The introduction of
SupportedTypeand the use ofStructuredQuestionTypein the Props interface improves type safety and makes the component's requirements more explicit.
27-28: LGTM! Well-implemented type guard.The type guard effectively narrows the
StructuredQuestionTypetoSupportedType, providing runtime type safety.
39-55: LGTM! Well-structured query implementations.The query implementations follow React Query best practices:
- Proper query keys including the id
- Conditional enabling based on type and id
- Isolated queries for different data types
src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx (2)
13-16: LGTM! Import organization improves readability.The grouped imports from the questionnaire types module enhance code organization.
269-269: Verify type assertion safety.While the type assertion improves type safety by ensuring the
typeprop matchesStructuredQuestionType, consider using a type guard instead of type assertion to validate the type at runtime.Let's verify the structure of
StructuredQuestionTypeand its usage:✅ Verification successful
Type assertion is appropriate and safe in this context
The type assertion is necessary and safe here because:
- It's converting from an optional
structured_typeto a requiredtypeprop- The component rendering logic ensures the type exists
StructuredQuestionTypeis a well-defined union of string literals🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of StructuredQuestionType # Check the type definition ast-grep --pattern 'type StructuredQuestionType = $_' # Check other usages of this type in the codebase rg -t typescript "StructuredQuestionType" -A 3 -B 3Length of output: 644
Script:
#!/bin/bash # Find the component's props and type usage context ast-grep --pattern 'interface $_ { $$$ type: $_ $$$ }' # Get more context around the type assertion rg -g "*.{ts,tsx}" "type={type as StructuredQuestionType}" -B 10 -A 5 # Look for any runtime validation rg -g "*.{ts,tsx}" "type.*StructuredQuestionType" -B 3 -A 3Length of output: 10963
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Refactor
StructuredQuestionTypeStyle