Fix: Display Care Loading Icon Instead of "No Assets Found" on Asset Tab Revisit on advanced filter #9205
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)src/components/Assets/AssetsList.tsx (3)
The addition of
The loading prop update maintains consistency with the main component's loading state management.
The updated condition Let's verify that this loading state pattern is consistent across similar components: ✅ Verification successfulThe previous ast-grep pattern might have failed due to syntax. Let's try using ripgrep to find loading state patterns in TypeScript/TSX files, focusing on conditional blocks that include loading checks and the Loading component. Let's try without file type restrictions since the tsx extension might not be recognized in the sandbox environment. Loading state pattern is consistent with codebase practices The updated condition
The implementation follows the established pattern for handling loading states and undefined data in the application, ensuring consistent user experience across components. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for similar loading state patterns in other list components
ast-grep --pattern 'if ($loading || !$data) {
$$$
}'
Length of output: 56 Script: #!/bin/bash
# Search for loading state patterns in list components using ripgrep
rg -U "if.*loading.*\{[\s\S]*?Loading" --type ts --type tsx -A 5
Length of output: 94 Script: #!/bin/bash
# Search for loading state patterns in all files
rg -U "if.*loading.*\{[\s\S]*?Loading" -A 5
Length of output: 10331 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: 0
🧹 Outside diff range and nitpick comments (4)
src/components/Patient/SampleDetails.tsx (1)
267-270: LGTM! Consider extracting the className to a constant.The addition of the
keyprop is a good practice for React list rendering. The visual styling improvements with margin and border enhance consistency.Consider extracting the className string to a constant or CSS module for better maintainability and reusability:
+const FLOW_CARD_CLASS_NAME = "mb-2 shadow-sm border border-secondary-300"; const renderFlow = (flow: FlowModel) => { return ( <Card key={flow.id} - className="mb-2 shadow-sm border border-secondary-300" + className={FLOW_CARD_CLASS_NAME} >src/components/Assets/AssetsList.tsx (3)
Line range hint
213-221: Revise the conditional logic to correctly handle loading and empty statesThe current condition
loading || !assetsExistwill show the loading indicator both during loading and when there are no assets. This doesn't align with the PR objective of only showing the loading state during data fetch.Apply this diff to fix the logic:
- if (loading || !assetsExist) { + if (loading) { manageAssets = ( <div className="col-span-3 w-full py-8 text-center"> <Loading /> </div> ); - } else if (assetsExist) { + } else if (!assetsExist) { + manageAssets = ( + <div className="col-span-3 w-full rounded-lg bg-white p-2 py-8 pt-4 text-center"> + <p className="text-2xl font-bold text-secondary-600">No Assets Found</p> + </div> + ); + } else {This change will:
- Show loading indicator only during the loading state
- Show "No Assets Found" when there are genuinely no assets
- Show the asset list when assets exist
Line range hint
1-600: Consider the following improvements for better maintainability and user experience
- Consolidate loading states:
- Currently using both
isLoadingandloadingstates which could lead to inconsistent UX- Consider using a single loading state manager or use more specific names like
isQRScanningLoading
- Enhance error handling for QR scanning:
- Add specific error messages for different failure scenarios
- Consider adding retry mechanism for failed scans
- Performance optimizations:
- Consider memoizing the asset list rendering
- Use
useCallbackfor handlers likeaccessAssetIdFromQRExample implementation for memoization:
import { useMemo, useCallback } from 'react'; // Memoize the asset list const renderAssetList = useMemo(() => ( <div className="grid grid-cols-1 gap-2 md:-mx-8 md:grid-cols-2 lg:grid-cols-3"> {assets.map((asset: AssetData) => ( // ... existing render logic ))} </div> ), [assets]); // Memoize the QR handler const handleQRScan = useCallback(async (assetURL: string) => { // ... existing QR handling logic }, []);
Line range hint
89-142: Enhance error handling in QR code scanningThe current error handling uses generic messages and doesn't distinguish between different error types. Consider implementing more specific error handling for better user feedback.
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_found"), }); 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_not_linked_to_qr"), }); } } else { setIsLoading(false); Notification.Error({ - msg: t("invalid_asset_id_msg"), + msg: t("missing_asset_id_in_qr"), }); } } catch (err) { + console.error('QR Scan Error:', err); Notification.Error({ - msg: t("invalid_asset_id_msg"), + msg: t("qr_scan_error"), }); } finally { setIsLoading(false); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Assets/AssetsList.tsx(1 hunks)src/components/Patient/SampleDetails.tsx(1 hunks)
|
Hey @Jacobjeevan i have done the required changes . Once look into it |
There was a problem hiding this comment.
This issue doesn't seem to be happening for tabs other than Assets tab. Most of the other tabs are having the similar implementation. I'd recommend actually debugging why that "no assets found" is coming in the first place as how it's mentioned in the issue.
Using timer to solve this is more of a hack that hides the root cause of the issue and is not the right solution.
Ideally the loading state provided by the useQuery should be enough to show the Loading state and if not loading, data.results.length being 0 would convey if it's actually empty assets for the applied filters.
rithviknishad
added
invalid
Yes, The issue is not occurring in other tabs; it is only happening on the Assets page. I have checked the code for the other tabs, and they have a similar implementation. However, in So, I have updated the
I have removed it
On page revisit, the data is initially undefined, which is why the issue is occurring. This condition was not properly handled |
rithviknishad
removed
invalid
|
@Jacobjeevan review required |
|
Closing as root issue was fixed in #9222 |
Proposed Changes
Fixes No Assets Found Message Displayed Instead of Loading Icon on Revisit of Assets Tab After Applying Advanced Filter #9202
Replaced the "No assets found" message with the Care loading icon when advanced filters are applied, ensuring better user experience.
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes