Migrate PaginatedList to useQuery
#10350
Conversation
WalkthroughThis pull request updates the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (7)
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/CAREUI/misc/PaginatedList.tsx (1)
74-84:useQueryusage is correctly keyed and configured.
Consider ensuringqueryParamsis a stable reference to avoid unintentional re-fetches if the object shape changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CAREUI/misc/PaginatedList.tsx(8 hunks)src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx(1 hunks)src/components/Patient/PatientDetailsTab/EncounterHistory.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (14)
src/CAREUI/misc/PaginatedList.tsx (12)
1-1: Well-adopted use ofreact-query.
No issues found with this import statement.
10-10: ImportingapiQueryis consistent with the new data-fetching logic.
No concerns.
11-15: Imported types reflect actual usage.
All imports (ApiRoute,PaginatedResponse,QueryParams) appear necessary and properly utilized.
20-25: CombiningUseQueryResultwith custom fields is appropriate.
Merging the query result with extra pagination metadata is a neat way to centralize context.
39-52:Props<TItem>interface is well-structured.
All prop definitions align with the newuseQueryapproach and provide clear optional fields.
60-65: Destructuring is clear and matches the refactored API.
No issues found; parameter naming and defaults look good.
67-67: InitializingcurrentPageproperly.
UsinginitialPage ?? 1is a straightforward defaulting approach.
71-71: Optional chaining foronPageChangeis safe and concise.
No further concerns.
112-112:WhenEmptylogic is direct and minimalistic.
The condition for skipping render when loading or non-empty is correct for an "empty" placeholder.Also applies to: 114-114
124-124:WhenLoadinggracefully handles the loading state.
Using!isLoadingto skip rendering is concise.Also applies to: 126-126
140-140:Refreshbutton usage is coherent.
Disabling while loading and the spinner class usage are consistent with best practices.Also applies to: 147-147, 151-151
203-203:Paginatorskip-while-loading is reasonable.
This prevents extraneous UI interactions until data is ready.Also applies to: 205-205
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx (1)
24-24: RenamingquerytoqueryParamsaligns with the updatedPaginatedListprops.
Good consistency in naming across the codebase.src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx (1)
285-285: UsingqueryParamsforPaginatedListmaintains consistency.
No issues found.
There was a problem hiding this comment.
let's delete this component altogether and instead directly implement things in the usages instead. building this component in the first place itself was a mistake 😅. so many things feels abstracted away and not very flexible.
There was a problem hiding this comment.
Sure, I am working on it.
will do it one by one in this PR itself.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx (1)
303-331: Consider extracting loading and empty states to reusable components.The loading and empty states implementation is good, but these patterns might be reused across the application.
Consider creating reusable components:
// src/components/Common/States/LoadingState.tsx export const LoadingState = ({ count = 3 }) => ( <div className="grid gap-5"> <CardListSkeleton count={count} /> </div> ); // src/components/Common/States/EmptyState.tsx export const EmptyState = ({ message }: { message: string }) => ( <Card className="p-6"> <div className="text-lg font-medium text-gray-500">{message}</div> </Card> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (4)
src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx (4)
1-1: LGTM! Import changes align with migration objectives.The new imports for
useQueryanduseFiltersare appropriate for the component's migration fromPaginatedListto a directuseQueryimplementation.Also applies to: 11-14
Line range hint
44-279: LGTM! Helper functions and sub-components are well-structured.The utility functions and sub-components maintain clear separation of concerns and handle complex data transformations effectively.
286-300: LGTM! Query implementation follows best practices.The useQuery implementation:
- Includes all necessary dependencies in queryKey
- Uses debounced query for performance
- Correctly calculates pagination offset
281-284: Verify the cache blacklist configuration.The
questionnaireparameter is blacklisted from cache. Please verify if this is intentional and document the reasoning.✅ Verification successful
Cache Blacklist Configuration Verified
- The only instance of blacklisting
"questionnaire"is found insrc/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx, suggesting it was intentionally added for this component.- This intentional exclusion likely prevents caching of dynamic query parameters to ensure the freshest data is fetched.
- No similar cache blacklist patterns for
"questionnaire"exist elsewhere in the codebase, which further supports the specificity of this configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of cache blacklisting to understand the pattern rg -A 2 "cacheBlacklist.*questionnaire" --type tsLength of output: 322
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx (1)
44-92: Consider simplifying the render logic.While the implementation is functional, the render logic could be improved:
- Multiple nested
divelements could be flattened- Conditional rendering could be simplified
- Empty state could be extracted into a reusable component
Here's a suggested refactor:
- <div className="mt-8"> - <div> - {encountersLoading ? ( - <div> - <div className="grid gap-5"> - <CardListSkeleton count={3} /> - </div> - </div> + <div className="mt-8"> + {encountersLoading ? ( + <div className="grid gap-5"> + <CardListSkeleton count={3} /> + </div> + ) : !encountersFetching && encounterData?.results?.length === 0 ? ( + <EmptyEncounterState + facilityId={facilityId} + patientId={patientId} + /> + ) : ( + <ul className="grid gap-4"> + {encounterData?.results?.map((encounter) => ( + <li key={encounter.id} className="w-full"> + <EncounterCard encounter={encounter} /> + </li> + ))} + <div className="flex w-full items-center justify-center"> + <Pagination totalCount={encounterData?.count || 0} /> + </div> + </ul> + )} + </div>And create a new component for the empty state:
const EmptyEncounterState = ({ facilityId, patientId }: { facilityId: string; patientId: string }) => { const { t } = useTranslation(); return ( <div className="p-2"> <div className="h-full space-y-2 rounded-lg bg-white px-7 py-12 border border-secondary-300"> <div className="flex w-full items-center justify-center text-lg text-secondary-600"> {t("no_consultation_history")} </div> <div className="flex w-full items-center justify-center pt-4"> <Button variant="outline_primary" asChild> <Link href={`/facility/${facilityId}/patient/${patientId}/consultation`}> <span className="flex w-full items-center justify-start gap-2"> <CareIcon icon="l-chat-bubble-user" className="text-xl" /> {t("add_consultation")} </span> </Link> </Button> </div> </div> </div> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx (3)
1-16: LGTM! Well-organized imports.The imports are logically grouped and include all necessary dependencies for the new implementation using
useQuery.
23-26: Resolve inconsistent limit values.There's a mismatch between the limit values:
useFiltersis configured withlimit: 15- The actual query uses
limit: 5(line 37)This inconsistency could lead to confusion. Consider aligning these values or documenting why they differ.
28-41: LGTM! Well-implemented query configuration.The implementation follows React Query best practices:
- Proper query key structure for cache management
- Debounced API calls to prevent request spam
- Comprehensive loading states extraction
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx (5)
28-41: Consider these improvements to the query configuration.
- The limit value (5) is duplicated in both
useFiltersand query parameters. Consider extracting it to a constant.- The offset calculation could be moved to a constant for better readability.
- Consider adding error handling for the debounced query.
Here's a suggested refactor:
+const ITEMS_PER_PAGE = 5; + const { qParams, Pagination, resultsPerPage } = useFilters({ - limit: 5, + limit: ITEMS_PER_PAGE, cacheBlacklist: ["encounterHistory"], }); +const currentOffset = ((qParams.page ?? 1) - 1) * resultsPerPage; + const { data: encounterData, isFetching: encountersFetching, isLoading: encountersLoading, + error, } = useQuery({ queryKey: ["encountersHistory", patientId, qParams], queryFn: query.debounced(routes.encounter.list, { queryParams: { patient: patientId, - limit: 5, - offset: ((qParams.page ?? 1) - 1) * resultsPerPage, + limit: ITEMS_PER_PAGE, + offset: currentOffset, }, }), + retry: 2, });
44-52: Simplify the nested div structure in the loading state.The loading state contains unnecessary nested divs that can be simplified.
-<div> - <div className="grid gap-5"> +<div className="grid gap-5"> <CardListSkeleton count={3} /> - </div> </div>
80-82: Remove duplicate key prop in EncounterCard.The
keyprop is unnecessarily duplicated on both theliandEncounterCardelements.<li key={encounter.id} className="w-full"> - <EncounterCard key={encounter.id} encounter={encounter} /> + <EncounterCard encounter={encounter} /> </li>
54-77: Consider extracting the empty state to a separate component.The empty state UI logic is quite complex and could be extracted into a reusable component, especially if similar empty states are used elsewhere in the application.
Example:
const EmptyEncounterState = ({ facilityId, patientId }: { facilityId: string; patientId: string }) => { const { t } = useTranslation(); return ( <div className="p-2"> <div className="h-full space-y-2 rounded-lg bg-white px-7 py-12 border border-secondary-300"> {/* ... rest of the empty state JSX ... */} </div> </div> ); };
1-92: Well-implemented migration to useQuery!The migration from
PaginatedListtouseQueryhas been executed effectively, with proper handling of different states and clean integration with theuseFiltershook. The component maintains good separation of concerns and follows React best practices.Consider these architectural improvements for the future:
- Extract reusable UI components for empty and loading states
- Implement error boundaries or error states for failed queries
- Consider implementing infinite scroll instead of pagination for a better user experience
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx (1)
1-26: LGTM! Clean imports and hook configuration.The imports are well-organized and the
useFiltershook is properly configured with appropriate pagination limits and cache settings.
|
questionnaire response encounter history resource comments |
…keleton loaders for Comments
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx (3)
23-26: Extract magic number and fix cache key inconsistency.Two issues to address:
- The limit value of 5 is hardcoded and repeated later in the query params. Consider extracting it to a constant.
- The cacheBlacklist uses "encounterHistory" while the query key uses "encountersHistory" (plural). This inconsistency might affect caching behavior.
+const ENCOUNTERS_PER_PAGE = 5; + const { qParams, Pagination, resultsPerPage } = useFilters({ - limit: 5, - cacheBlacklist: ["encounterHistory"], + limit: ENCOUNTERS_PER_PAGE, + cacheBlacklist: ["encountersHistory"], });
28-42: Use the extracted constant for consistency.The query configuration looks good, with proper key structure and debounced API calls. However, the limit value is hardcoded again.
queryFn: query.debounced(routes.encounter.list, { queryParams: { patient: patientId, - limit: 5, + limit: ENCOUNTERS_PER_PAGE, offset: ((qParams.page ?? 1) - 1) * resultsPerPage, }, }),
79-83: Remove duplicate key prop.The key prop is unnecessarily duplicated on both the
liandEncounterCardelements.{encounterData?.results?.map((encounter) => ( <li key={encounter.id} className="w-full"> - <EncounterCard key={encounter.id} encounter={encounter} /> + <EncounterCard encounter={encounter} /> </li> ))}src/components/Resource/ResourceCommentSection.tsx (1)
24-43: LGTM! Query configuration follows best practices.The implementation correctly:
- Uses array-based query key for proper cache invalidation
- Implements pagination with offset calculation
- Uses debounced query function
Consider extracting the magic number
15into a constant:+const COMMENTS_PER_PAGE = 15; + const { qParams, Pagination, resultsPerPage } = useFilters({ - limit: 15, + limit: COMMENTS_PER_PAGE, cacheBlacklist: ["resourceComments"], }); const { data: resourceComments, isFetching: commentsFetching, isLoading: commentsLoading, refetch: refetchComments, } = useQuery({ queryKey: [routes.getResourceComments.path, id, qParams], queryFn: query.debounced(routes.getResourceComments, { queryParams: { - limit: 15, + limit: COMMENTS_PER_PAGE, offset: ((qParams.page ?? 1) - 1) * resultsPerPage, }, pathParams: { id }, }), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Facility/models.tsx(1 hunks)src/components/Patient/PatientDetailsTab/EncounterHistory.tsx(1 hunks)src/components/Resource/ResourceCommentSection.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Resource/ResourceCommentSection.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
🔇 Additional comments (6)
src/components/Patient/PatientDetailsTab/EncounterHistory.tsx (2)
1-17: LGTM! Well-organized imports.The imports are logically grouped and all dependencies are necessary for the component's functionality.
44-92: LGTM! Well-structured render logic.The component handles all states effectively:
- Loading state with skeleton UI
- Empty state with clear user action
- List rendering with semantic HTML
- Proper pagination integration
src/components/Facility/models.tsx (1)
44-44: Verify the impact of CommentModel changes.The type change from
numbertoUserBareMinimumforcreated_byis a breaking change that might affect other components usingCommentModel.Run the following script to find all usages of
CommentModel:src/components/Resource/ResourceCommentSection.tsx (3)
1-1: LGTM! Imports and hook usage align with migration requirements.The changes correctly implement the migration requirements from the learnings:
- Using
useQueryfrom@tanstack/react-query- Adding loading state components
- Using the
useFiltershook for paginationAlso applies to: 10-12
86-111: LGTM! UI states are handled appropriately.The implementation correctly handles:
- Loading state with skeleton UI
- Empty state with user-friendly message
- List rendering with pagination
120-144: LGTM! Comment component implementation is clean and complete.The component correctly:
- Uses the updated
CommentModelwithUserBareMinimumtype- Formats comment text and timestamps
- Displays user information with avatar
| onClick={async () => { | ||
| await onSubmitComment(); | ||
| refetchComments(); | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in comment submission.
The comment submission and refetch operations should be wrapped in a try-catch block to handle potential failures gracefully.
Apply this diff to improve error handling:
-onClick={async () => {
- await onSubmitComment();
- refetchComments();
-}}
+onClick={async () => {
+ try {
+ await onSubmitComment();
+ await refetchComments();
+ } catch (error) {
+ toast.error(t("failed_to_update_comments"));
+ }
+}}📝 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.
| onClick={async () => { | |
| await onSubmitComment(); | |
| refetchComments(); | |
| }} | |
| onClick={async () => { | |
| try { | |
| await onSubmitComment(); | |
| await refetchComments(); | |
| } catch (error) { | |
| toast.error(t("failed_to_update_comments")); | |
| } | |
| }} |
Proposed Changes
useTanStackQueryInsteadtouseQueryinPaginatedList.tsx@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
useFiltershook to streamline data handling.Refactor
useQueryfor data fetching across multiple components, improving performance and maintainability.