Fix: Layout update of Asset Page #9466
Conversation
WalkthroughThe pull request updates the 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 (2)
src/components/Assets/AssetsList.tsx (2)
434-448: Consider utilizing multi-field search capabilityWhile the implementation is functional, the
SearchByMultipleFieldscomponent is currently used with only one search field despite its capability to handle multiple fields.Consider adding more search fields to enhance the search functionality:
<SearchByMultipleFields id="asset-search" options={[ { key: "Name/ Serial no./ QR code ID", label: "name/serial no./QR code ID", type: "text" as const, placeholder: "Search by name/serial no./QR code ID", value: qParams.search || "", shortcutKey: "f", }, + { + key: "location", + label: "Location", + type: "text" as const, + placeholder: "Search by location", + value: qParams.location || "", + }, + { + key: "status", + label: "Status", + type: "select" as const, + placeholder: "Filter by status", + value: qParams.status || "", + options: [ + { label: "Working", value: "working" }, + { label: "Not Working", value: "not_working" }, + ], + }, ]} className="w-full" onSearch={(key, value) => updateQuery({ [key]: value })} clearSearch={clearSearch} />
Line range hint
543-595: Enhance warrantyAmcValidityChip implementationThe function logic is sound, but could benefit from some improvements.
Consider these enhancements:
+const WARRANTY_EXPIRY_THRESHOLDS = { + CRITICAL: 30, + WARNING: 90, +} as const; + +const getDaysDifference = (date1: Date, date2: Date) => + Math.ceil(Math.abs(Number(date1) - Number(date2)) / (1000 * 60 * 60 * 24)); + export const warrantyAmcValidityChip = ( warranty_amc_end_of_validity: string, ) => { if (warranty_amc_end_of_validity === "" || !warranty_amc_end_of_validity) return; const today = new Date(); const warrantyAmcEndDate = new Date(warranty_amc_end_of_validity); - const days = Math.ceil( - Math.abs(Number(warrantyAmcEndDate) - Number(today)) / - (1000 * 60 * 60 * 24), - ); + const days = getDaysDifference(today, warrantyAmcEndDate); if (warrantyAmcEndDate < today) { return ( <Chip id="warranty-amc-expired-red" variant="danger" startIcon="l-times-circle" text="AMC/Warranty Expired" /> ); - } else if (days <= 30) { + } else if (days <= WARRANTY_EXPIRY_THRESHOLDS.CRITICAL) { return ( <Chip id="warranty-amc-expiring-soon-orange" variant="custom" className="border-orange-300 bg-orange-100 text-orange-900" startIcon="l-exclamation-circle" text="AMC/Warranty Expiring Soon" /> ); - } else if (days <= 90) { + } else if (days <= WARRANTY_EXPIRY_THRESHOLDS.WARNING) { return ( <Chip id="warranty-amc-expiring-soon-yellow" variant="warning" startIcon="l-exclamation-triangle" text="AMC/Warranty Expiring Soon" /> ); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Assets/AssetsList.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Assets/AssetsList.tsx (2)
Line range hint 30-42: LGTM: Clean import additions and hook update
The new imports and hook updates are well-structured and maintain consistency with the codebase.
Line range hint 359-423: LGTM: Well-structured filter and export functionality
The advanced filter and import/export menu implementation is clean, with proper authorization checks and comprehensive export options.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Assets/AssetsList.tsx (1)
Line range hint
533-589: Handle edge cases in warranty validationThe warranty validation logic should handle invalid date formats and consider timezone implications.
export const warrantyAmcValidityChip = ( warranty_amc_end_of_validity: string, ) => { if (warranty_amc_end_of_validity === "" || !warranty_amc_end_of_validity) return; + + // Validate date format + const warrantyAmcEndDate = new Date(warranty_amc_end_of_validity); + if (isNaN(warrantyAmcEndDate.getTime())) { + console.error(`Invalid warranty date format: ${warranty_amc_end_of_validity}`); + return; + } + const today = new Date(); - const warrantyAmcEndDate = new Date(warranty_amc_end_of_validity); + // Set time to start of day to avoid timezone issues + today.setHours(0, 0, 0, 0); + warrantyAmcEndDate.setHours(0, 0, 0, 0); const days = Math.ceil( Math.abs(Number(warrantyAmcEndDate) - Number(today)) / (1000 * 60 * 60 * 24), );
🧹 Nitpick comments (1)
src/components/Assets/AssetsList.tsx (1)
431-445: Consider expanding search capabilitiesWhile the SearchByMultipleFields component is a good addition, consider adding more commonly searched fields to improve user experience.
options={[ { key: "Name/ Serial no./ QR code ID", label: "name/serial no./QR code ID", type: "text" as const, placeholder: "Search by name/serial no./QR code ID", value: qParams.search || "", shortcutKey: "f", }, + { + key: "Asset Class", + label: "asset class", + type: "text" as const, + placeholder: "Search by asset class", + value: qParams.asset_class || "", + }, + { + key: "Status", + label: "status", + type: "text" as const, + placeholder: "Search by status", + value: qParams.status || "", + }, ]}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Assets/AssetsList.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Assets/AssetsList.tsx (2)
30-31: LGTM: Clean import declarations
The new imports are properly organized and follow the project's import structure.
Line range hint 322-420: Simplify nested layout structure
The button layout still has unnecessary nesting that can be simplified for better maintainability.
Consider this simplified structure:
-<div className="flex flex-wrap items-center gap-3">
- <div className="mb-2 flex w-full flex-col items-center gap-3 lg:mb-0 lg:w-fit lg:flex-row lg:gap-5">
- <div className="w-full lg:w-fit">
+<div className="flex flex-wrap items-center gap-3">
+ <div className="flex w-full flex-wrap items-center gap-3 lg:w-fit">
<Button
variant="primary"
size="lg"
className="w-full p-[10px] md:w-auto"
onClick={() => setIsScannerActive(true)}
>
<CareIcon icon="l-search" className="text-base mr-2" />
Scan Asset QR
</Button>
- </div>
- <div className="w-full lg:w-fit" data-testid="create-asset-button">
<Button
variant="primary"
size="lg"
disabled={!NonReadOnlyUsers}
className="w-full p-[10px] md:w-auto"
+ data-testid="create-asset-button"
onClick={() => {
if (qParams.facility) {
navigate(`/facility/${qParams.facility}/assets/new`);
} else {
setShowFacilityDialog(true);
}
}}
>
<CareIcon icon="l-plus-circle" className="text-lg mr-2" />
<span>{t("create_asset")}</span>
</Button>
- </div>
</div>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Assets/AssetsList.tsx (2)
432-446: LGTM: Well-implemented search functionality with accessibilityThe SearchByMultipleFields implementation is solid with proper configuration and accessibility support via shortcut key.
Consider adding an aria-label to improve screen reader support:
<SearchByMultipleFields id="asset-search" + aria-label="Search assets by name, serial number, or QR code ID" options={[
Line range hint
533-589: LGTM: Well-implemented warranty validity status logicThe warrantyAmcValidityChip function provides clear visual feedback with proper date handling.
Consider memoizing the chip components to prevent unnecessary re-renders:
+const ExpiredChip = memo(() => ( + <Chip + id="warranty-amc-expired-red" + variant="danger" + startIcon="l-times-circle" + text="AMC/Warranty Expired" + /> +)); export const warrantyAmcValidityChip = ( warranty_amc_end_of_validity: string, ) => { if (warranty_amc_end_of_validity === "" || !warranty_amc_end_of_validity) return; const today = new Date(); const warrantyAmcEndDate = new Date(warranty_amc_end_of_validity); const days = Math.ceil( Math.abs(Number(warrantyAmcEndDate) - Number(today)) / (1000 * 60 * 60 * 24), ); if (warrantyAmcEndDate < today) { - return ( - <Chip - id="warranty-amc-expired-red" - variant="danger" - startIcon="l-times-circle" - text="AMC/Warranty Expired" - /> - ); + return <ExpiredChip />; } // Similar changes for other chips...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/pageobject/Asset/AssetCreation.ts(1 hunks)src/components/Assets/AssetsList.tsx(4 hunks)
🔇 Additional comments (3)
cypress/pageobject/Asset/AssetCreation.ts (1)
5-5: LGTM: Selector update improves specificity and fixes typo
The change from [data-testid=create-asset-buttom] button to #create-asset-button is an improvement as it:
- Uses a more specific and performant ID selector
- Fixes the typo in the old selector ("buttom")
- Correctly maps to the button's ID in the UI component
✅ Verification successful
LGTM: Selector change is correctly implemented and matches UI component
The ID selector #create-asset-button in the Cypress test correctly corresponds to the button element in src/components/Assets/AssetsList.tsx where the ID is properly implemented. The change improves test reliability by using a unique, specific selector.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the ID selector exists in the codebase
rg -l 'id="create-asset-button"'
Length of output: 71
Script:
#!/bin/bash
# Let's verify the actual implementation in AssetsList.tsx to ensure the ID is used correctly
rg -A 2 'id="create-asset-button"' src/components/Assets/AssetsList.tsx
Length of output: 213
src/components/Assets/AssetsList.tsx (2)
30-31: LGTM: Clean integration of new components and hooks
The new imports and hook updates properly support the UI enhancements.
Also applies to: 42-42
322-355: Consider simplifying the nested layout structure
The button layout still has unnecessary nesting that could be simplified while maintaining the same functionality.
|
I don’t think the Cypress tests failed due to the changes in this PR. Could someone confirm? |
Jacobjeevan
added
needs testing
needs review
and removed
question
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Assets/AssetsList.tsx (2)
432-446: Enhance search capabilitiesThe SearchByMultipleFields component is currently configured with only one search option. Consider adding more relevant search fields to fully utilize its capabilities.
options={[ { key: "Name/ Serial no./ QR code ID", label: "name/serial no./QR code ID", type: "text" as const, placeholder: "Search by name/serial no./QR code ID", value: qParams.search || "", shortcutKey: "f", }, + { + key: "location", + label: "Location", + type: "text" as const, + placeholder: "Search by location", + value: qParams.location || "", + }, + { + key: "asset_class", + label: "Asset Class", + type: "text" as const, + placeholder: "Search by asset class", + value: qParams.asset_class || "", + } ]}
Line range hint
533-599: Improve date handling and maintainabilityThe warrantyAmcValidityChip function could benefit from the following improvements:
- Extract magic numbers as named constants
- Simplify date difference calculation
- Add JSDoc documentation for better maintainability
+const EXPIRY_THRESHOLDS = { + CRITICAL: 30, + WARNING: 90, +} as const; + +/** + * Generates a chip component based on warranty/AMC validity status + * @param warranty_amc_end_of_validity - The end date of warranty/AMC + * @returns A Chip component indicating the warranty/AMC status + */ export const warrantyAmcValidityChip = ( warranty_amc_end_of_validity: string, ) => { if (warranty_amc_end_of_validity === "" || !warranty_amc_end_of_validity) return; const today = new Date(); const warrantyAmcEndDate = new Date(warranty_amc_end_of_validity); - const days = Math.ceil( - Math.abs(Number(warrantyAmcEndDate) - Number(today)) / - (1000 * 60 * 60 * 24), - ); + const days = Math.ceil((warrantyAmcEndDate.getTime() - today.getTime()) / (1000 * 60 * 60 * 24)); if (warrantyAmcEndDate < today) { return ( <Chip id="warranty-amc-expired-red" variant="danger" startIcon="l-times-circle" text="AMC/Warranty Expired" /> ); - } else if (days <= 30) { + } else if (days <= EXPIRY_THRESHOLDS.CRITICAL) { return ( <Chip id="warranty-amc-expiring-soon-orange" variant="custom" className="border-orange-300 bg-orange-100 text-orange-900" startIcon="l-exclamation-circle" text="AMC/Warranty Expiring Soon" /> ); - } else if (days <= 90) { + } else if (days <= EXPIRY_THRESHOLDS.WARNING) { return ( <Chip id="warranty-amc-expiring-soon-yellow" variant="warning" startIcon="l-exclamation-triangle" text="AMC/Warranty Expiring Soon" /> ); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Assets/AssetsList.tsx(4 hunks)
🔇 Additional comments (1)
src/components/Assets/AssetsList.tsx (1)
322-363: Simplify nested layout structure
The button layout has unnecessary nesting and duplicate flex containers that can be simplified.
…ub.com/AdityaJ2305/care_fe into issue/9349/button_layout_in_asset_page
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Assets/AssetsList.tsx (1)
322-363: 🛠️ Refactor suggestionSimplify nested layout structure
The button layout still has unnecessary nesting that can be simplified. Consider the previous suggestion to reduce the nesting levels.
Apply this diff to simplify the layout:
- <div className="flex flex-wrap items-center gap-3"> - <div className="mb-2 flex w-full flex-col items-center gap-3 lg:mb-0 lg:w-fit lg:flex-row lg:gap-5"> - <div className="w-full lg:w-fit"> + <div className="flex flex-wrap items-center gap-3"> + <div className="flex w-full flex-wrap items-center gap-3 lg:w-fit"> <Button variant="primary" size="lg" className="w-full p-[10px] md:w-auto" onClick={() => setIsScannerActive(true)} > <CareIcon icon="l-search" className="text-base mr-2" /> Scan Asset QR </Button> - </div> - <div className="w-full lg:w-fit"> <Button variant="primary" size="lg" id="create-asset-button" disabled={!NonReadOnlyUsers} className="w-full p-[10px] md:w-auto" onClick={() => { if (qParams.facility) { navigate(`/facility/${qParams.facility}/assets/new`); } else { setShowFacilityDialog(true); } }} > <CareIcon icon="l-plus-circle" className="text-lg mr-2" /> <span>{t("create_asset")}</span> </Button> - </div> </div>
🧹 Nitpick comments (2)
src/components/Assets/AssetsList.tsx (2)
424-431: Improve responsive layout for count blockThe count block's empty className prop and the flex layout could be improved for better responsiveness.
Apply this diff:
- <div className="mt-4 gap-4 lg:gap-16 flex flex-col lg:flex-row lg:items-center"> + <div className="mt-4 flex flex-col gap-4 lg:flex-row lg:items-center lg:gap-16"> <CountBlock text="Total Assets" count={totalCount} loading={loading} icon="d-folder" - className="" + className="w-full lg:w-auto" />
436-442: Improve search field label clarityThe search field's key and label could be more consistent and clearer.
Apply this diff:
{ - key: "Name/ Serial no./ QR code ID", - label: "name/serial no./QR code ID", + key: "asset_search", + label: "Name / Serial No. / QR Code ID", type: "text" as const, - placeholder: "Search by Name / Serial no. / QR code ID", + placeholder: "Search by Name / Serial No. / QR Code ID", value: qParams.search || "", shortcutKey: "f", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Assets/AssetsList.tsx(4 hunks)
🔇 Additional comments (3)
src/components/Assets/AssetsList.tsx (3)
11-12: LGTM: Import changes align with component updates
The new imports for Button and SearchByMultipleFields components are correctly structured and follow the absolute import pattern.
Also applies to: 19-19
Line range hint 530-589: LGTM: Well-structured warranty/AMC validation logic
The warrantyAmcValidityChip function is well-implemented with clear conditions and appropriate visual indicators for different warranty states.
42-42: Verify search functionality regression
The new SearchByMultipleFields component and clearSearch hook integration look good, but we should verify that the search functionality works as expected with the new implementation.
Run the following script to check for any existing tests covering the search functionality:
Also applies to: 432-446
✅ Verification successful
Search functionality is well-tested and implemented correctly
The search functionality has been thoroughly verified through the following findings:
- The
SearchByMultipleFieldscomponent is properly integrated with appropriate props including theclearSearchfunctionality - The
useFiltershook correctly implements theclearSearchstate management - Comprehensive Cypress test coverage exists in
cypress/e2e/assets_spec/that specifically tests:- Search by asset name
- Search by QR code
- Search by serial number
- Filter badge presence verification
- Search result verification
The implementation and test coverage indicate that the search functionality is working as expected with the new changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for search-related test coverage
rg -A 5 "asset-search|SearchByMultipleFields" --type=spec --type=test
Length of output: 100
Script:
#!/bin/bash
# Let's try a broader search for test files first
fd "test|spec" --type f
# Then search for any test files related to assets or search
rg -l "AssetsList|SearchByMultipleFields|clearSearch" --type=typescript
Length of output: 264
Script:
#!/bin/bash
# Search for test files with .test or .spec extensions
fd "\.(test|spec)\.(ts|tsx|js|jsx)$"
# Search for any files containing our search terms without file type restriction
rg -l "AssetsList|SearchByMultipleFields|clearSearch"
# Look for the implementation of SearchByMultipleFields to understand the search functionality
ast-grep --pattern 'const SearchByMultipleFields = $_'
Length of output: 500
Script:
#!/bin/bash
# Let's examine the implementation of SearchByMultipleFields and its usage in AssetsList
rg -A 10 "SearchByMultipleFields" src/components/Common/SearchByMultipleFields.tsx
rg -A 10 "SearchByMultipleFields" src/components/Assets/AssetsList.tsx
# Check the useFilters hook implementation since it's related to clearSearch
rg -A 10 "clearSearch" src/hooks/useFilters.tsx
# Look for any Cypress tests that might cover asset search functionality
rg -A 10 "asset.*search" cypress/e2e --ignore-case
Length of output: 10244
rithviknishad
removed
needs review
Deploy-Failed
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Common/Export.tsx (3)
3-3: Prefer consistent import usage throughout the codebase.
You introduced the new Button import, but the file still references ButtonV2. Consider using one button component for clarity.
84-84: Nitpick: Remove extra classes or ensure consistent styling.
The className merges border, text, and hover classes. Confirm that this doesn't conflict with overall theming.
135-135: Move away from inline color naming in class names.
"bg-red" can be replaced with a token-based utility class if your design system supports it.src/components/Assets/AssetsList.tsx (1)
322-356: Simplify nested containers.
Deeply nested flex containers might be reduced. This also mirrors older feedback suggesting simpler layout structures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CAREUI/interactive/FiltersSlideover.tsx(2 hunks)src/components/Assets/AssetsList.tsx(4 hunks)src/components/Common/Export.tsx(5 hunks)
🔇 Additional comments (8)
src/components/Common/Export.tsx (1)
Line range hint 54-74: Style and variant usage look clean.
Thanks for aligning these button props with the new design system. The variant="outline_primary" works well with the "Exporting..." text.
src/CAREUI/interactive/FiltersSlideover.tsx (2)
7-8: Good to see the new Button usage.
This maintains consistency with the rest of the app’s UI library.
63-71: Renaming "advanced_filters" to "filters" is consistent with user feedback.
The reduced wording helps the UI remain concise.
src/components/Assets/AssetsList.tsx (5)
11-19: Excellent substitution of SearchByMultipleFields and the new Button.
Both changes enhance the interface and improve usability.
357-363: “Filters” button usage is a great improvement.
It’s simpler than the old “Advanced Filter” label, matching the naming guidelines suggested in PR comments.
Line range hint 366-421: Tooltip usage.
Check if the tooltip fully references the new design system. Some design tokens might differ for tooltips.
✅ Verification successful
Let me search for tooltip-related implementations and design system references to verify this concern.
Let me check the design system's tooltip implementation specifically.
Tooltip implementation uses current design system tokens
The tooltip implementation in src/style/CAREUI.css already follows the design system's patterns using Tailwind classes and design tokens:
- Uses
@applydirectives with design system tokens likebg-black/75,text-white,rounded - Implements consistent spacing, transitions, and z-index management
- Properly handles backdrop blur and opacity
The tooltip in AssetsList.tsx is using this standard implementation from the design system, and no changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for tooltip class usage and implementation
rg -l "className=\"tooltip\""
# Search for tooltip-related styles or design tokens
rg "tooltip" -g "*.css" -g "*.scss"
# Search for design system tooltip components
ast-grep --pattern 'import { $_ } from "@care/$_"'
Length of output: 1226
Script:
#!/bin/bash
# Check the tooltip implementation in CAREUI.css
cat src/style/CAREUI.css | grep -A 10 "\.tooltip"
# Search for any new design system tooltip components
rg -l "Tooltip" --type typescript
# Check if there's a dedicated tooltip component in the design system
fd -e tsx -e ts -p "Tooltip" src/CAREUI/
Length of output: 1211
424-449: Ensure thorough testing of new search capabilities.
With the new “clearSearch” function, confirm each search param is cleared as intended.
✅ Verification successful
Let me gather information about the search functionality and the clearSearch implementation.
Let me search for the implementation of clearSearch and the query parameter handling to understand how the search clearing works.
Let me try to find the useFilters hook implementation which seems to be the core of the search functionality.
Based on the comprehensive code review, I can now provide the final response:
Search functionality is properly implemented with clear parameter handling
The search functionality is well-implemented with proper clearing mechanism:
- The
clearSearchstate inuseFiltershook properly manages clearing search parameters - The
SearchByMultipleFieldscomponent correctly handles the clearing throughuseEffectthat watchesclearSearchchanges - The search field properly resets when
clearSearchis triggered through theremoveFiltersfunction
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find the clearSearch implementation and related search functionality
ast-grep --pattern 'const clearSearch = $_'
# Find any tests related to search functionality
fd "test|spec" -e ts -e tsx -e js -e jsx --exec rg -l "clearSearch|SearchByMultipleFields"
# Look for other usages of SearchByMultipleFields to understand the pattern
rg "SearchByMultipleFields" -A 5 -B 5
Length of output: 9208
Script:
#!/bin/bash
# Find clearSearch implementation in AssetsList.tsx
rg -B 10 -A 2 "clearSearch" src/components/Assets/AssetsList.tsx
# Look for updateQuery implementation which is used in the search
rg -B 5 -A 5 "updateQuery" src/components/Assets/AssetsList.tsx
# Check the useFilters hook implementation since it's imported and likely handles the search state
rg -B 10 -A 10 "useFilters" src/hooks/useFilters.ts
Length of output: 1472
Script:
#!/bin/bash
# Find useFilters hook implementation
fd useFilters -t f --exec rg -l "export.*useFilters"
# Look for the actual implementation
fd useFilters -t f --exec cat {}
# Check SearchByMultipleFields component implementation
cat src/components/Common/SearchByMultipleFields.tsx
Length of output: 17476
42-42: clearSearch
Ensure it resets all fields consistently, including advanced filters.
✅ Verification successful
Let me search for the implementation and usage of clearSearch to verify its behavior.
Let me search for the implementation of advanced filters to verify how they interact with the search clearing mechanism.
Based on the analysis of the codebase, I can now generate the final response:
clearSearch implementation is consistent with advanced filters
The implementation in useFilters hook shows that clearSearch is properly integrated with the filter management system. When clearSearch is triggered:
- It's handled by
SearchByMultipleFieldscomponent which clears the search input - The
useFiltershook manages both search and advanced filters separately, ensuring they don't interfere with each other - Advanced filters are managed through the
advancedFilterprop and have their own clear mechanism viaremoveFilters
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find the implementation of clearSearch
ast-grep --pattern 'clearSearch = $_'
# Also search for any direct assignments or declarations
ast-grep --pattern 'const clearSearch = $_'
# Search for any function declarations
ast-grep --pattern 'function clearSearch($_) { $$$ }'
# Search for any method declarations
ast-grep --pattern 'clearSearch($_) { $$$ }'
# Backup search using ripgrep for any mentions
rg "clearSearch" -A 5
Length of output: 7223
Script:
#!/bin/bash
# Search for AssetFilter implementation
ast-grep --pattern 'const AssetFilter = $_'
# Search for the filter implementation in the Assets directory
rg -A 10 "AssetFilter" src/components/Assets/
# Look for filter-related state management
rg "useFilters" -A 10
Length of output: 30521
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/pageobject/utils/advanceFilterHelpers.ts(1 hunks)src/CAREUI/interactive/FiltersSlideover.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CAREUI/interactive/FiltersSlideover.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Common/Export.tsx(6 hunks)src/components/Common/Menu.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/Common/Export.tsx
[error] 114-114: React Hook "useIsAuthorized" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function.
(react-hooks/rules-of-hooks)
🪛 GitHub Check: lint
src/components/Common/Export.tsx
[failure] 114-114:
React Hook "useIsAuthorized" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function
🔇 Additional comments (5)
src/components/Common/Menu.tsx (1)
79-86: New type definition "ShadcnMenuDropdownItemProps" looks good.
This type provides flexibility for dropdown items, especially with the optional "onClick" and "icon" fields.
src/components/Common/Export.tsx (4)
20-20: Good update to the "ExportItem" interface.
Switching from "DropdownItemProps" to "ShadcnMenuDropdownItemProps" correctly aligns the export menu items with the new dropdown structure.
Line range hint 57-77: Single-item export flow is well handled.
The logic to decide and invoke the export action (if present) is straightforward and consistent with the existing flow.
🧰 Tools
🪛 eslint
[error] 114-114: React Hook "useIsAuthorized" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function.
(react-hooks/rules-of-hooks)
🪛 GitHub Check: lint
[failure] 114-114:
React Hook "useIsAuthorized" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function
85-94: Dropdown trigger usage looks clean.
Using "asChild" ensures the Button is rendered properly while avoiding nested button issues. The layout is also consistent, showing the export icon and label.
Line range hint 141-166: ExportButton modifications are consistent.
Replacing "ButtonV2" with the new "Button" while preserving export logic and interface is well executed.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/components/Common/Export.tsx (1)
Line range hint
150-194: Refactor duplicate export logic and migrate to shadcn ButtonThe export handling logic is duplicated between
ExportButtonandExportMenu. Additionally, this component still usesButtonV2while the rest of the file uses shadcn'sButton.
- Extract the common export logic into a shared function
- Migrate to the shadcn
Buttoncomponent for consistencyExample refactor:
// Extract shared logic const createExportAction = (action: ExportItem['action'], route?: Route<string | { results: object[] }, unknown>) => { if (route) { return async () => { try { const { data } = await request(route); return data ?? null; } catch (error) { console.error('Export request failed:', error); return null; } }; } return action; }; // Use in ExportButton <Button disabled={isExporting || props.disabled} onClick={() => { const action = createExportAction(props.action, props.route); if (action) { exportFile(action, props.filenamePrefix, type, parse); } }} size="icon" variant="outline" className="mx-2" > {isExporting ? ( <CareIcon icon="l-spinner-alt" className="animate-spin" /> ) : ( <CareIcon icon="l-export" /> )} <span className={`tooltip-text ${tooltipClassName}`}> {props.tooltip || "Export"} </span> </Button>
🧹 Nitpick comments (1)
src/components/Common/Export.tsx (1)
Line range hint
99-119: Add error handling for failed export requestsThe export functionality makes API requests but doesn't handle potential failures. Consider adding error handling and user feedback.
Example improvement:
onClick={() => { let action = item.action; if (item.route) { action = async () => { - const { data } = await request(item.route!); - return data ?? null; + try { + const { data } = await request(item.route!); + return data ?? null; + } catch (error) { + console.error('Export request failed:', error); + // Add toast notification or other user feedback + return null; + } }; } if (action) { exportFile(action, item.filePrefix, item.type, item.parse); } }}Also applies to: 125-145
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/Export.tsx(4 hunks)
🧰 Additional context used
🪛 eslint
src/components/Common/Export.tsx
[error] 60-60: React Hook "useIsAuthorized" is called conditionally. React Hooks must be called in the exact same order in every component render.
(react-hooks/rules-of-hooks)
🪛 GitHub Check: lint
src/components/Common/Export.tsx
[failure] 60-60:
React Hook "useIsAuthorized" is called conditionally. React Hooks must be called in the exact same order in every component render
|
| @@ -4,6 +4,8 @@ import { useTranslation } from "react-i18next"; | |||
| import CareIcon from "@/CAREUI/icons/CareIcon"; | |||
| import SlideOver from "@/CAREUI/interactive/SlideOver"; | |||
|
|
|||
| import { Button } from "@/components/ui/button"; | |||
|
|
|||
There was a problem hiding this comment.
Let's remove the ButtonV2 import below (and make sure to sub out any uses of ButtonV2).
There was a problem hiding this comment.
This file is using ButtonV2 here 👇
Export.tsx - Line 109
Using the Button from Shadcn breaks the UI on other pages like the resource and shifting pages. Here’s an image showing the issue (button beside the title) 👇
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Common/Export.tsx (1)
Line range hint
99-119: Consider reusing ExportMenuItem logic for consistency.The single item case could be refactored to reuse the ExportMenuItem logic to maintain consistency and reduce code duplication.
- <Button - disabled={isExporting || disabled} - size="default" - onClick={() => { - let action = item.action; - if (item.route) { - action = async () => { - const { data } = await request(item.route!); - return data ?? null; - }; - } - if (action) { - exportFile(action, item.filePrefix, item.type, item.parse); - } - }} - variant="outline_primary" - className="py-2.5" - > - <CareIcon icon="l-export" className="mr-1" /> - {isExporting ? "Exporting..." : label} - </Button> + <Button + disabled={isExporting || disabled} + size="default" + variant="outline_primary" + className="py-2.5" + onClick={() => { + const menuItem = document.createElement('div'); + const exportMenuItem = <ExportMenuItem item={item} exportFile={exportFile} />; + ReactDOM.render(exportMenuItem, menuItem); + menuItem.querySelector('button')?.click(); + }} + > + <CareIcon icon="l-export" className="mr-1" /> + {isExporting ? "Exporting..." : label} + </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/Export.tsx(4 hunks)
🔇 Additional comments (5)
src/components/Common/Export.tsx (5)
3-12: LGTM: Import statements follow the absolute import pattern.
The new shadcn UI component imports are correctly structured using absolute imports.
22-22: LGTM: Interface update aligns with the new shadcn components.
The ExportItem interface has been updated to use ShadcnMenuDropdownItemProps.
47-87: LGTM: ExportMenuItem implementation follows React best practices.
The component correctly:
- Uses hooks at the top level
- Provides a fallback for authorization
- Handles both action and onClick scenarios properly
125-145: LGTM: Clean implementation of shadcn dropdown menu.
The dropdown implementation correctly uses shadcn components and properly maps ExportMenuItem components.
Line range hint 151-190: Complete the migration to shadcn Button component.
The ExportButton component still uses ButtonV2. For consistency with the rest of the codebase, migrate it to use the shadcn Button component.
- <ButtonV2
- disabled={isExporting || props.disabled}
- onClick={() => {
- let action = props.action;
- if (props.route) {
- action = async () => {
- const { data } = await request(props.route!);
- return data ?? null;
- };
- }
- if (action) {
- exportFile(action, props.filenamePrefix, type, parse);
- }
- }}
- className="tooltip mx-2 p-4 text-lg text-secondary-800 disabled:bg-transparent disabled:text-secondary-500"
- variant="secondary"
- ghost
- circle
- >
+ <Button
+ disabled={isExporting || props.disabled}
+ onClick={() => {
+ let action = props.action;
+ if (props.route) {
+ action = async () => {
+ const { data } = await request(props.route!);
+ return data ?? null;
+ };
+ }
+ if (action) {
+ exportFile(action, props.filenamePrefix, type, parse);
+ }
+ }}
+ className="tooltip mx-2 p-4 text-lg text-secondary-800 disabled:bg-transparent disabled:text-secondary-500"
+ variant="ghost"
+ size="icon"
+ >
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Screenshots
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes