Added skeleton loader during advanced filters #9020
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Assessment against linked issues
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. |
Rishith25
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
src/CAREUI/display/Count.tsx (1)
Line range hint
1-33: Consider adding prop-types validation and documentation.Since this is a shared UI component (CAREUI), consider the following improvements:
- Add JSDoc comments documenting the null behavior
- Add runtime validation for the count prop
- Update component tests to cover null scenarios
Would you like me to help generate the documentation and test cases?
src/components/Facility/FacilityUsers.tsx (2)
275-279: LGTM! The changes effectively prevent showing 0 during loading.The implementation correctly handles the loading state by passing
nullto trigger the skeleton loader, which aligns with the PR objectives to fix the count display during data fetching.Consider simplifying the condition and adding proper TypeScript types:
- count={ - isLoading || facilityUserData?.count === undefined - ? null - : facilityUserData.count - } + type FacilityUserData = { + count: number; + results: Array<any>; // Define proper user type + }; + + count={isLoading ? null : facilityUserData?.count}This refactor:
- Adds type safety with proper TypeScript interfaces
- Simplifies the condition since
undefinedcheck is handled by optional chaining
275-279: Consider consolidating loading state handling.The component has multiple loading indicators:
- CountBlock's
loadingprop- CountBlock's
countbeingnull<Loading />component whenfacilityUserDatais nullThis might lead to inconsistent loading states. Consider using a single source of truth for loading state.
Suggestions:
- Use only the
isLoadingstate from useQuery- Remove the separate
loadingprop from CountBlock since thenullcount already triggers the skeleton- Use Suspense boundaries for consistent loading behavior across the app
src/components/Facility/DischargedPatientsList.tsx (1)
284-286: LGTM! The loading state implementation looks good.The conditional rendering of
nullfor the count prop effectively prevents the display of zero counts during loading states. This implementation aligns well with the PR objective of improving the user experience during data fetching.Consider extracting the condition into a more descriptive variable for improved readability:
-count={ - facilityQuery.loading || count === undefined ? null : count -} +const isLoadingCount = facilityQuery.loading || count === undefined; +count={isLoadingCount ? null : count}src/components/Assets/AssetsList.tsx (2)
Line range hint
31-32: Consider consolidating loading state management.The component currently manages multiple loading states (
loadingfrom useQuery andisLoadingfor QR scanning). Consider unifying these into a single loading state manager for better maintainability and consistency.Example approach:
// Create a custom hook for managing loading states const useLoadingState = () => { const [loadingStates, setLoadingStates] = useState({ dataFetching: false, qrScanning: false }); const setLoading = (key: keyof typeof loadingStates, value: boolean) => setLoadingStates(prev => ({ ...prev, [key]: value })); const isLoading = Object.values(loadingStates).some(Boolean); return { loadingStates, setLoading, isLoading }; };Also applies to: 362-362, 427-429
Line range hint
108-157: Enhance error handling in QR code scanning.The current error handling uses generic messages for different error scenarios. Consider providing more specific error messages for different failure cases to improve user experience.
Example improvement:
const accessAssetIdFromQR = async (assetURL: string) => { try { setIsLoading(true); setIsScannerActive(false); if (!isValidURL(assetURL)) { setIsLoading(false); Notification.Error({ - msg: t("invalid_asset_id_msg"), + msg: t("invalid_qr_url_format"), }); return; } const params = parseQueryParams(assetURL); const assetId = params.asset || params.assetQR; if (assetId) { const { data } = await request(routes.listAssetQR, { pathParams: { qr_code_id: assetId }, }); if (!data) { setIsLoading(false); Notification.Error({ - msg: t("invalid_asset_id_msg"), + msg: t("qr_code_not_registered"), }); return; } const { data: assetData } = await request(routes.listAssets, { query: { qr_code_id: assetId, limit: 1 }, }); if (assetData?.results.length === 1) { navigate( `/facility/${assetData.results[0].location_object.facility?.id}/assets/${assetData.results[0].id}`, ); } else { setIsLoading(false); Notification.Error({ - msg: t("asset_not_found_msg"), + msg: t("asset_deleted_or_unavailable"), }); } } else { setIsLoading(false); Notification.Error({ - msg: t("invalid_asset_id_msg"), + msg: t("missing_asset_id_in_qr"), }); } } catch (err) { Notification.Error({ - msg: t("invalid_asset_id_msg"), + msg: t("qr_scan_failed"), }); } finally { setIsLoading(false); } };src/components/Users/ManageUsers.tsx (3)
Line range hint
279-309: Improve type safety in weekly hours validation.The current type conversion using the
+operator could lead to NaN in edge cases. Consider using a more robust parsing approach.- if (!username || !weeklyHours || +weeklyHours < 0 || +weeklyHours > 168) { + const hours = Number(weeklyHours); + if (!username || !weeklyHours || Number.isNaN(hours) || hours < 0 || hours > 168) { setWeeklyHoursError("Value should be between 0 and 168"); return; }
Line range hint
654-662: Standardize loading indicators across the component.The component uses different loading indicators (
LoadingandCircularProgress) in different places. Consider standardizing the loading UI pattern for consistency.Also applies to: 787-793
Line range hint
1-1000: Consider restructuring the component for better maintainability.The current file structure has several areas for improvement:
- The file contains two large components that could be split into separate files
- Helper functions could be extracted to a separate utilities file
- Consider using a reducer or custom hook to manage related state
src/components/Patient/ManagePatients.tsx (1)
977-979: LGTM! The count handling aligns with the PR objectives.The implementation correctly prevents showing a misleading zero count during loading by passing
nullto theCountBlockcomponent. This change effectively addresses issue #8030.Consider adding type safety by explicitly typing the count prop:
- count={ - isLoading || data?.count === undefined ? null : data?.count - } + count={ + isLoading || data?.count === undefined ? null : (data.count as number) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/CAREUI/display/Count.tsx(2 hunks)src/components/Assets/AssetsList.tsx(1 hunks)src/components/Facility/DischargedPatientsList.tsx(1 hunks)src/components/Facility/FacilityList.tsx(1 hunks)src/components/Facility/FacilityUsers.tsx(1 hunks)src/components/Patient/ManagePatients.tsx(1 hunks)src/components/Users/ManageUsers.tsx(1 hunks)
🔇 Additional comments (5)
src/CAREUI/display/Count.tsx (2)
5-5: LGTM! Type change aligns with loading state requirements.
The change from number to number | null is a good approach for handling loading states, allowing explicit distinction between "no data yet" (null) and "zero items" (0).
Line range hint 23-29: Verify loading state behavior across components.
The loading state implementation looks good and addresses the original issue of showing 0 counts during loading. However, we should verify that all parent components are correctly passing null during loading states.
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient information about how the CountBlock component is being used across the codebase.
Loading states are properly handled across all parent components
All instances of CountBlock usage in the codebase correctly handle loading states by passing null during loading or when count is undefined:
- ManageUsers.tsx:
count={userListLoading || districtDataLoading || userListData?.count === undefined ? null : ...} - ManagePatients.tsx:
count={isLoading || data?.count === undefined ? null : data?.count} - FacilityUsers.tsx:
count={isLoading || facilityUserData?.count === undefined ? null : facilityUserData.count} - FacilityList.tsx:
count={isLoading || permittedData?.count === undefined ? null : permittedData.count} - DischargedPatientsList.tsx:
count={facilityQuery.loading || count === undefined ? null : count} - AssetsList.tsx:
count={loading || totalCount === undefined ? null : totalCount}
The only minor exception is in SampleViewAdmin.tsx which uses count={sampeleData?.count || 0}, but it properly passes the loading state separately via the loading prop.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CountBlock usage patterns across the codebase
# Expected: All CountBlock usages should handle loading states by passing null
# Search for CountBlock usage patterns
rg -A 5 "CountBlock" | grep -v "src/CAREUI/display/Count.tsx"
# Search specifically for count prop patterns to verify null handling
ast-grep --pattern 'count={$_}'
Length of output: 7453
src/components/Facility/FacilityList.tsx (1)
181-185: LGTM! Verify CountBlock's null handling.
The conditional rendering of count aligns well with the PR objectives by preventing the display of zero counts during loading states. The implementation correctly handles both loading and undefined states.
Let's verify that the CountBlock component properly handles null values:
✅ Verification successful
CountBlock properly handles null values
The CountBlock component is correctly implemented to handle null values by showing a loading skeleton when either loading is true or count is null. The changes in FacilityList.tsx to pass null during loading states align with the component's design.
Reasoning:
- The CountBlock Props interface explicitly types
countasnumber | null - The component implementation shows proper null handling:
{props.loading || props.count == null ? (
<dd className="h-10 w-full max-w-[100px] animate-pulse rounded-lg bg-secondary-300" />
) : (
<dd id="count" className="text-5xl font-black leading-9">
{props.count}
</dd>
)}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CountBlock component's handling of null values
# Test: Look for CountBlock's implementation and its handling of null counts
ast-grep --pattern 'interface Props {
$$$
count: $count_type
$$$
}'
# Test: Look for conditional rendering based on count being null
ast-grep --pattern 'const CountBlock = $$$({count}$$$) => {
$$$
}'
Length of output: 146
Script:
#!/bin/bash
# Let's try a broader search for CountBlock component and its props
rg -A 10 "CountBlock" --type typescript
# Also search for any type definitions related to count prop
rg "count.*:" --type typescript
# Search for any imports of CountBlock to understand its location
rg "import.*CountBlock" --type typescript
Length of output: 222
Script:
#!/bin/bash
# Let's try searching without type filters since typescript type isn't recognized
# Search for CountBlock component definition and usage
rg -A 10 "CountBlock"
# Search for files that might contain CountBlock
fd CountBlock
# Look for type definitions or interfaces containing count
rg -A 5 "interface.*Props"
Length of output: 98943
Script:
#!/bin/bash
# Let's look at the CountBlock component implementation in CAREUI
rg -A 10 "interface Props" src/CAREUI/display/Count.tsx
# Also check for any usages of CountBlock with null values
rg "count={.*null.*}" --type ts --type tsx
Length of output: 455
Script:
#!/bin/bash
# Search for CountBlock usages with null values using a simpler pattern
rg "count=\{.*\?.*null" --type ts --type tsx
# Look at the implementation of CountBlock to see how it handles null
rg -A 15 "export default function CountBlock" src/CAREUI/display/Count.tsx
Length of output: 934
src/components/Assets/AssetsList.tsx (1)
362-362: LGTM! The count prop update improves loading state handling.
The change correctly implements the loading state for the count display, preventing the misleading "0" count during data fetching. This aligns well with the PR objectives.
src/components/Users/ManageUsers.tsx (1)
547-553: LGTM! Implementation successfully addresses the loading state issue.
The conditional logic effectively prevents displaying a zero count during loading by passing null when the data is being fetched or is undefined. This directly addresses the PR objective of implementing a loading state for patient counts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/components/Patient/SampleViewAdmin.tsx (1)
Line range hint
1-424: Consider these improvements for better consistency and error handling.While the loading state implementation for CountBlock is good, there are a few areas where the component could be enhanced:
- Loading state handling could be unified across the component
- Error states for data fetching should be handled explicitly
- Complex computations in the render method could benefit from memoization
Consider these improvements:
export default function SampleViewAdmin() { + // Memoize the filtered and mapped sample list + const memoizedSampleList = useMemo(() => { + if (!sampeleData?.results) return []; + return sampeleData.results.map((item) => { + // ... existing mapping logic + }); + }, [sampeleData?.results]); // ... existing code if (isLoading || !sampeleData) { manageSamples = ( <div className="flex w-full justify-center"> <Loading /> </div> ); + } else if (error) { + manageSamples = ( + <div className="w-full rounded-lg bg-white p-3"> + <div className="mt-4 flex w-full justify-center text-2xl font-bold text-red-600"> + Error loading sample tests + </div> + </div> + ); } else if (sampeleData?.count) { manageSamples = ( <> - {sampleList} + {memoizedSampleList} <Pagination totalCount={sampeleData?.count} /> </> ); }src/components/Assets/AssetsList.tsx (3)
74-74: Consider explicit null handling for undefined dataWhile optional chaining is good, consider explicitly setting
totalCounttonullwhendatais undefined for more predictable behavior.- setTotalCount(data?.count); + setTotalCount(data?.count ?? null);
362-364: Simplify loading state handlingThe current condition is a bit complex and has some redundancy with the separate
loadingprop. Consider simplifying:- count={loading || totalCount === undefined ? null : totalCount} - loading={loading} + count={totalCount} + loading={loading}The
CountBlockcomponent can handle the null state internally along with the loading prop for a cleaner implementation.
446-446: Consider showing skeleton loader for PaginationWhile checking for
totalCount !== nullprevents invalid rendering, consider showing a skeleton loader for the Pagination component during loading to prevent layout shifts.- {totalCount !== null && <Pagination totalCount={totalCount} />} + {loading ? ( + <div className="h-10 w-full animate-pulse rounded bg-gray-100" /> + ) : ( + totalCount !== null && <Pagination totalCount={totalCount} /> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Assets/AssetsList.tsx(4 hunks)src/components/Patient/SampleViewAdmin.tsx(1 hunks)
🔇 Additional comments (2)
src/components/Patient/SampleViewAdmin.tsx (1)
338-342: LGTM! The count prop implementation effectively prevents premature zero counts.
The conditional logic properly handles loading states and undefined data, ensuring users don't see misleading zero counts while data is being fetched. This implementation aligns well with the PR objective of adding a skeleton loader during advanced filters.
src/components/Assets/AssetsList.tsx (1)
44-44: LGTM: Improved type safety for loading state
The change to allow null values for totalCount is a good approach to represent the loading state.
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation