Optimize Pagination | Switch to TanStackQuery in ResourcesList | Prevent API calls | Fix Translation
#10273
Conversation
WalkthroughThis pull request introduces optimizations to data fetching across multiple components using TanStack Query. The changes primarily focus on improving user experience during data loading by implementing Changes
Assessment against linked issues
Possibly related PRs
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/pages/Facility/FacilitiesPage.tsx (1)
62-63: LGTM! Consider standardizing staleTime format.The optimization changes look good and work well with the
enabledcondition. However, for consistency across the codebase, consider standardizing the staleTime format:- staleTime: 60 * 1000, + staleTime: 1000 * 60,src/pages/Facility/FacilityDetailsPage.tsx (1)
52-53: Consider applying optimizations to both queries.While the optimization for the users query is good, the facility query could benefit from the same optimizations:
const { data: facilityResponse, isLoading } = useQuery<FacilityModel>({ queryKey: ["facility", id], queryFn: query(routes.getAnyFacility, { pathParams: { id }, }), + placeholderData: keepPreviousData, + staleTime: 60 * 1000, });src/pages/Organization/OrganizationFacilities.tsx (1)
51-52: LGTM! Consider standardizing staleTime format.The optimization changes look good and work well with the
enabledcondition. However, for consistency across the codebase, consider standardizing the staleTime format:- staleTime: 60 * 1000, + staleTime: 1000 * 60,src/pages/Organization/OrganizationPatients.tsx (1)
86-87: LGTM! Good optimization of data fetching.The addition of
keepPreviousDataandstaleTimeoptions improves the user experience by:
- Preventing loading flickers during pagination
- Reducing unnecessary API calls with a 60-second stale time window
Consider extracting the stale time duration to a constant since it's used across multiple components.
+// Add to a constants file +export const DEFAULT_QUERY_STALE_TIME = 60 * 1000; - staleTime: 60 * 1000, + staleTime: DEFAULT_QUERY_STALE_TIME,src/components/Files/FilesTab.tsx (1)
111-113: LGTM! Good optimization with appropriate stale time for files.The changes improve the component by:
- Preventing unnecessary API calls when
typeis not available- Using a longer stale time (5 minutes) which is appropriate for files that change less frequently
- Preventing loading flickers with
keepPreviousDataConsider adding a comment explaining why files use a longer stale time compared to other resources.
+ // Files are cached longer (5 minutes) as they change less frequently than other resources staleTime: 1000 * 60 * 5,src/pages/Encounters/EncounterList.tsx (1)
Line range hint
175-183: Consider applying similar optimizations to the single encounter query.For consistency, consider adding
keepPreviousDataandstaleTimeto the single encounter query as well.const { data: queryEncounter } = useQuery<Encounter>({ queryKey: ["encounter", encounter_id], queryFn: query(routes.encounter.get, { pathParams: { id: encounter_id }, queryParams: { facility: facilityId, }, }), enabled: !!encounter_id, + placeholderData: keepPreviousData, + staleTime: 60 * 1000, });src/components/Resource/ResourceList.tsx (1)
59-64: Add error handling for CSV download query.Consider handling potential download failures to improve user experience.
const { data: csvFile } = useQuery({ queryKey: ["downloadResourcesCsv", appliedFilters], queryFn: query(routes.downloadResourceRequests, { queryParams: { ...appliedFilters, csv: true }, }), + retry: 2, + onError: (error) => { + // Show error notification to user + console.error('Failed to download CSV:', error); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
public/locale/en.json(2 hunks)src/components/Facility/FacilityUsers.tsx(2 hunks)src/components/Files/FilesTab.tsx(2 hunks)src/components/Resource/ResourceList.tsx(7 hunks)src/pages/Encounters/EncounterList.tsx(2 hunks)src/pages/Facility/FacilitiesPage.tsx(2 hunks)src/pages/Facility/FacilityDetailsPage.tsx(2 hunks)src/pages/Organization/OrganizationFacilities.tsx(2 hunks)src/pages/Organization/OrganizationPatients.tsx(2 hunks)
🔇 Additional comments (5)
src/components/Facility/FacilityUsers.tsx (1)
46-47: LGTM! Optimization improves UX during pagination.The addition of
keepPreviousDataandstaleTimeoptions effectively optimizes the data fetching strategy:
- Prevents UI flicker during pagination by retaining previous data
- Reduces unnecessary API calls with a 1-minute stale time window
src/pages/Encounters/EncounterList.tsx (1)
172-173: LGTM! Good optimization of encounter list fetching.The addition of
keepPreviousDataandstaleTimeoptions improves the user experience by preventing loading flickers during filtering and reducing unnecessary API calls.src/components/Resource/ResourceList.tsx (2)
1-1: Excellent implementation of TanStack Query!The query configuration demonstrates best practices:
- Using
keepPreviousDataprevents UI flicker during refetches- Setting
staleTime: 1000 * 60 * 2(2 minutes) reduces unnecessary API calls- Query key properly includes dependencies (
qParams)Also applies to: 42-57
213-213: LGTM! Clean implementation of export functionality.The ExportButton action is simplified to directly use the query result.
public/locale/en.json (1)
580-580: LGTM! Translation entries added correctly.The new entries support the translation key changes in the codebase:
- Added "consent_status" to match the updated key in ResourceList.tsx
- Added error message for user data fetching
Also applies to: 925-925
…hub.com/rajku-dev/care_fe into issue/10220/optimize-pagination-queries
|
@rithviknishad Please review the changes. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/Resource/ResourceList.tsx (1)
279-281: Optimize list rendering performance.Consider memoizing the resource list rendering to prevent unnecessary re-renders.
+ const memoizedResourceList = useMemo( + () => showResourceCardList(resourcesList?.results || []), + [resourcesList?.results] + ); - <div>{showResourceCardList(resourcesList?.results || [])}</div> + <div>{memoizedResourceList}</div>Don't forget to add the import:
+ import { useMemo } from 'react';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(2 hunks)src/components/Resource/ResourceList.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/components/Resource/ResourceList.tsx (2)
1-1: LGTM! Clean import of TanStack Query utilities.The imports are well-organized and correctly include the necessary TanStack Query hooks and utilities.
Also applies to: 22-22
Line range hint
270-270: Translation key inconsistency detected.The translation key
consent__statusis still being used while other parts of the codebase have moved toconsent_status. This inconsistency will cause translation issues.This was previously flagged in earlier reviews. Please ensure consistent usage of translation keys across the entire application.
| const { data: csvFile } = useQuery({ | ||
| queryKey: ["downloadResourcesCsv", appliedFilters], | ||
| queryFn: query(routes.downloadResourceRequests, { | ||
| queryParams: { ...appliedFilters, csv: true }, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve CSV download implementation.
The current CSV download implementation has several potential issues:
- Missing error handling for failed downloads
- No loading state indication
- Potential null reference when csvFile is undefined
const {
data: csvFile,
+ isLoading: isDownloading,
+ error: downloadError
} = useQuery({
queryKey: ["downloadResourcesCsv", appliedFilters],
queryFn: query(routes.downloadResourceRequests, {
queryParams: { ...appliedFilters, csv: true },
}),
+ retry: 2,
});
// In ExportButton
<ExportButton
variant="secondary"
className="ml-4 bg-transparent shadow-none text-black rounded-full"
- action={async () => csvFile ?? null}
+ action={async () => {
+ if (downloadError) {
+ throw new Error(t("error_downloading_csv"));
+ }
+ return csvFile ?? null;
+ }}
+ loading={isDownloading}
filenamePrefix="resource_requests"
/>Also applies to: 213-213
…hub.com/rajku-dev/care_fe into issue/10220/optimize-pagination-queries
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/CAREUI/misc/PaginatedList.tsx (1)
Line range hint
185-198: Consider using optional chaining for better null safety.The type simplification looks good, but the null check for
data?.countcould be more consistent.Apply this diff to improve null safety:
- if (hideIfSinglePage && (data?.count ?? 0) <= perPage) { + if (hideIfSinglePage && data?.count <= perPage) { return null; } return ( <Pagination className={className} cPage={currentPage} - data={{ totalCount: data?.count ?? 0 }} + data={{ totalCount: data?.count || 0 }} defaultPerPage={perPage} onChange={setPage} /> );This change:
- Uses a more idiomatic approach with optional chaining
- Uses
||instead of??since we specifically want to handle bothnullandundefinedsrc/Utils/request/useQuery.ts (2)
42-42: Consider making keepPreviousData configurable.While
keepPreviousDatais excellent for pagination, applying it globally might not be optimal for all queries. Consider making it configurable through the options parameter:export interface QueryOptions<TData> extends RequestOptions<TData> { prefetch?: boolean; key?: string; + keepPreviousData?: boolean; } // In the useQuery config: - placeholderData: keepPreviousData, + placeholderData: options?.keepPreviousData ? keepPreviousData : undefined,
Line range hint
13-16: Enhance deprecation notice with migration examples.The current deprecation notice could be more helpful by including:
- A brief example of how to migrate to direct TanStack Query usage
- A link to migration documentation
- The benefits of migrating (e.g., direct access to TanStack Query features)
/** - * @deprecated use `useQuery` from `@tanstack/react-query` instead. + * @deprecated Use `useQuery` from `@tanstack/react-query` instead. + * Example migration: + * ```typescript + * // Before: + * const { data, loading } = useTanStackQueryInstead(route, options); + * + * // After: + * const { data, isLoading } = useQuery({ + * queryKey: [route.path, options?.pathParams], + * queryFn: () => request(route, options), + * placeholderData: keepPreviousData, + * }); + * ``` + * See our migration guide: <add_link_here> */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CAREUI/misc/PaginatedList.tsx(2 hunks)src/Utils/request/useQuery.ts(2 hunks)src/components/Resource/ResourceCommentSection.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- src/components/Resource/ResourceCommentSection.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/CAREUI/misc/PaginatedList.tsx (1)
157-159: LGTM! Improved loading state handling.The simplified rendering condition aligns well with TanStack Query's
keepPreviousDatapattern, providing a better user experience by maintaining previous data visibility during loading states.Consider implementing a minimum height container or consistent item heights to prevent layout shifts when data length changes. Run this script to check for potential layout shift issues:
✅ Verification successful
Layout shifts are properly managed ✓
The codebase implements consistent layout management through:
- Fixed container heights with proper overflow handling
- Uniform grid gaps and flex layouts
- Viewport-based height constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for height-related CSS classes or styles in components using PaginatedList rg -l "PaginatedList" | xargs rg "height|min-h"Length of output: 47
Script:
#!/bin/bash # First, find files containing PaginatedList echo "=== Files using PaginatedList ===" rg -l "PaginatedList" echo -e "\n=== Checking for CSS files ===" fd -e css -e scss echo -e "\n=== Checking for Tailwind classes in PaginatedList components ===" rg "className.*(?:h-|min-h-|max-h-)" -A 2 -B 2 $(rg -l "PaginatedList") echo -e "\n=== Checking for style props in PaginatedList components ===" rg "style.*(?:height|minHeight|maxHeight)" -A 2 -B 2 $(rg -l "PaginatedList")Length of output: 477
Script:
#!/bin/bash # Check for flex/grid layouts and container styles in PaginatedList and its usage echo "=== Checking PaginatedList implementation ===" rg "className.*(?:flex|grid|container)" -A 2 -B 2 src/CAREUI/misc/PaginatedList.tsx echo -e "\n=== Checking PaginatedList usage for container styles ===" rg "(?:<PaginatedList.*>|className.*(?:flex|grid|container))" -A 3 -B 3 \ src/components/Resource/ResourceCommentSection.tsx \ src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx \ src/components/Patient/PatientDetailsTab/EncounterHistory.tsx \ src/components/Patient/PatientDetailsTab/patientUpdates.tsxLength of output: 18702
src/Utils/request/useQuery.ts (1)
1-1: Consider migration strategy impact.While adding
keepPreviousDataimproves the functionality, adding new features to a deprecated utility might discourage migration to direct TanStack Query usage. Consider documenting the migration path to help users transition to the recommended approach.Let's check how widely this deprecated utility is still used:
…hub.com/rajku-dev/care_fe into issue/10220/optimize-pagination-queries
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/pages/Organization/OrganizationView.tsx (2)
1-1: LGTM! Well-implemented query optimizations.Good use of TanStack Query features:
keepPreviousDataprevents UI flicker during paginationstaleTimeof 1 minute reduces unnecessary API calls- Proper extraction of loading states
Consider adding
retry: falseto the query config if this is a user-initiated search, as retrying failed searches might not be desirable from a UX perspective.Also applies to: 35-50
Line range hint
31-34: Consider these improvements to the component logic.While the implementation is solid, here are some suggestions:
- The search input could benefit from debouncing to prevent excessive API calls during typing
- Consider moving the hardcoded
limit = 12to a constants file for better maintainabilityExample implementation:
+ import { useDebouncedCallback } from 'use-debounce'; + import { PAGINATION_LIMITS } from '@/constants'; - const limit = 12; // 3x4 grid + const limit = PAGINATION_LIMITS.ORGANIZATION_GRID; + const debouncedSetSearch = useDebouncedCallback( + (value: string) => { + setSearchQuery(value); + setPage(1); + }, + 300 + ); <Input placeholder="Search by name..." value={searchQuery} - onChange={(e) => { - setSearchQuery(e.target.value); - setPage(1); - }} + onChange={(e) => debouncedSetSearch(e.target.value)} className="w-full" />Also applies to: 71-77
src/pages/Organization/OrganizationUsers.tsx (2)
40-44: Remove unusedisFetchingUsersvariable.The
isFetchingUsersvariable is only used in theEntityBadgecomponent. Consider usingisLoadingconsistently throughout the component for better maintainability.const { data: users, - isFetching: isFetchingUsers, isLoading, } = useQuery({Then update the
EntityBadgecomponent:<EntityBadge title={t("users")} count={users?.count} - isFetching={isFetchingUsers} + isFetching={isLoading} translationParams={{ entity: "User" }} />
Line range hint
110-114: Match loading skeleton layout with actual data grid.The loading skeleton uses a different grid layout compared to the actual data. Consider matching the layouts for a smoother transition:
- <div className="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-4"> + <div className="grid grid-cols-1 md:grid-cols-2 xl:grid-cols-3 gap-4"> <CardGridSkeleton count={6} /> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/pages/Organization/OrganizationFacilities.tsx(4 hunks)src/pages/Organization/OrganizationPatients.tsx(4 hunks)src/pages/Organization/OrganizationUsers.tsx(4 hunks)src/pages/Organization/OrganizationView.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/Organization/OrganizationFacilities.tsx
- src/pages/Organization/OrganizationPatients.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/pages/Organization/OrganizationView.tsx (1)
Line range hint
83-87: Great improvement in loading state handling!Using
isLoadinginstead ofisFetchingfor skeleton display is the correct approach as it prevents unnecessary UI flicker during background updates while still showing loading states for initial data fetch.src/pages/Organization/OrganizationUsers.tsx (2)
1-1: LGTM! Good addition ofkeepPreviousData.The import of
keepPreviousDataaligns with the optimization strategy for pagination.
55-56: LGTM! Effective query optimization implementation.The addition of
placeholderData: keepPreviousDataandstaleTime: 1000 * 60will:
- Prevent UI flicker during pagination by retaining previous data
- Reduce unnecessary API calls by caching data for 1 minute
|
Can we migrate to |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Localization
Performance Improvements
keepPreviousDatato maintain smooth UI transitions.staleTimeto optimize data caching and reduce unnecessary network requests.Bug Fixes
The changes focus on improving application performance, user experience, and error handling across various components.