updated facilitypopup #1689
updated facilitypopup #1689
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several enhancements to the user interface and functionality of the facility management components. Key changes include the addition of new CSS classes in Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/SearchJurisdiction.js (1)
Line range hint
71-85: Consider adding aria-label for accessibility.The MultiSelectDropdown would benefit from an aria-label to improve accessibility for screen readers.
Add an aria-label to the MultiSelectDropdown:
<MultiSelectDropdown + aria-label={t("SELECT_BOUNDARIES")} variant={"nestedmultiselect"} selected={selectedBoundaries}health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FacilityPopup.js (1)
Line range hint
279-286: Enhance error handling in API mutationsThe current error handling could be improved to provide better debugging information and user feedback.
Consider adding error details and logging:
onError: async (result) => { - setShowToast({ key: "error", label: t("ERROR_WHILE_UPDATING_PLANFACILITY"), transitionTime: 5000 }); + console.error('Plan facility update failed:', result); + setShowToast({ + key: "error", + label: t("ERROR_WHILE_UPDATING_PLANFACILITY") + (result?.message ? `: ${result.message}` : ''), + transitionTime: 5000 + }); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/facility.scss (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FacilityPopup.js (6 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/SearchJurisdiction.js (3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FacilityPopup.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/SearchJurisdiction.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FacilityPopup.js
[error] 326-326: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 356-356: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
🔇 Additional comments (6)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/SearchJurisdiction.js (4)
7-7: LGTM: Function signature updated correctly.The addition of the style prop with a default empty object follows React best practices.
54-54: LGTM: Style prop applied correctly.The style prop is properly applied to the wrapper div, allowing for flexible styling from parent components.
57-57: LGTM: CardLabel styling is appropriate.Setting width:100% ensures the label spans the full container width, improving layout consistency.
7-7: Verify style prop usage in parent components.Let's verify how this component is being used to ensure the style prop is being utilized effectively.
✅ Verification successful
Style prop is being used correctly in parent components
The verification shows that the
styleprop is being utilized appropriately:
- In
FacilityPopup.js, it's used with specific styling:style={{ padding: "0px" }}- Other usages in
PopInbox.jsandPlanInbox.jsdon't pass the optional style prop, which is fine since it defaults to an empty object{}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SearchJurisdiction usage to verify style prop implementation rg -l "SearchJurisdiction" | xargs rg -A 5 "<SearchJurisdiction"Length of output: 2459
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FacilityPopup.js (2)
3-3: LGTM: SVG import additionThe SVG component is appropriately imported and used for visual feedback in the selection state UI.
187-192: LGTM: Improved row selection handlingThe refactored selection handling is more efficient and maintainable:
- Directly extracts IDs from event.selectedRows
- Properly manages both individual and bulk selection states
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/facility.scss
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/facility.scss
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/facility.scss
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/SearchJurisdiction.js
Outdated
Show resolved
Hide resolved
...h/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FacilityPopup.js
Outdated
Show resolved
Hide resolved
...h/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FacilityPopup.js
Show resolved
Hide resolved
Swathi-eGov
dismissed stale reviews from coderabbitai[bot] and ashish-egov
via
4779fa8
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (3)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/timelinePopUpWrapper.js (2)
Line range hint
8-54: Consider performance and error handling improvements.The component could benefit from some architectural improvements:
- Memoize the steps transformation to prevent unnecessary recalculations:
+ const transformSteps = React.useCallback((instances) => { + return instances.map((instance, index) => ({ + label: t(instance?.state?.applicationStatus || 'NA'), + variant: 'completed', + subElements: [ + Digit.Utils.microplanv1.epochToDateTime(instance?.auditDetails?.lastModifiedTime), + instance?.assignes?.length > 0 ? instance.assignes[0]?.name : 'NA', + instance?.comment ? t(instance.comment) : '' + ], + showConnector: true + })); + }, [t]); useEffect(() => { if (workflowData && workflowData.ProcessInstances) { - const steps = workflowData.ProcessInstances.map... + const steps = transformSteps(workflowData.ProcessInstances); setTimelineSteps(steps); } }, [workflowData]);
- Add error handling:
const TimelinePopUpWrapper = ({ onClose, businessId, heading }) => { + const [error, setError] = useState(null); // ... existing code ... return ( <PopUp onClose={onClose} heading={t(heading)} onOverlayClick={onClose} children={[ + error ? ( + <div className="error-message">{t('ERROR_LOADING_WORKFLOW')}</div> + ) : isLoading ? <Loader /> : <TimelineMolecule ... /> ]} /> ); };
Line range hint
7-7: Add PropTypes and JSDoc documentation.This component would benefit from proper documentation and type checking:
+ import PropTypes from 'prop-types'; + /** + * TimelinePopUpWrapper - A component that displays workflow timeline in a popup + * @param {Object} props + * @param {Function} props.onClose - Callback function to close the popup + * @param {string} props.businessId - Business ID for workflow lookup + * @param {string} props.heading - Heading text for the popup + * @returns {React.ReactElement} + */ const TimelinePopUpWrapper = ({ onClose, businessId, heading }) => { // ... existing code ... }; + TimelinePopUpWrapper.propTypes = { + onClose: PropTypes.func.isRequired, + businessId: PropTypes.string.isRequired, + heading: PropTypes.string.isRequired + };health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/SearchJurisdiction.js (1)
Line range hint
41-49: Consider adding error handling for boundary selection.The
handleBoundarySelectfunction could benefit from additional error handling for malformed event objects.Consider updating the handler with better error checking:
const handleBoundarySelect = (selectBoundariesEvent) => { - if (!selectBoundariesEvent) + if (!selectBoundariesEvent || !Array.isArray(selectBoundariesEvent)) return if (selectBoundariesEvent.length === 0) { setSelectedBoundaries([]) return } const boundariesInEvent = selectBoundariesEvent?.map(event => { + if (!Array.isArray(event) || event.length < 2) return null; return event?.[1] - }) + }).filter(Boolean) setSelectedBoundaries(boundariesInEvent) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FacilityPopup.js (6 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/SearchJurisdiction.js (3 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/timelinePopUpWrapper.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FacilityPopup.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/SearchJurisdiction.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/timelinePopUpWrapper.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FacilityPopup.js
[error] 326-326: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 356-356: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
🔇 Additional comments (8)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/timelinePopUpWrapper.js (1)
Line range hint
36-41: Add safety checks for nested properties and review translation key usage.While the addition of translations is good, there are a few potential issues to address:
Consider applying these improvements:
const steps = workflowData.ProcessInstances.map((instance, index) => ({ - label: t(instance.state.applicationStatus), + label: t(instance?.state?.applicationStatus || 'NA'), variant: 'completed', subElements: [Digit.Utils.microplanv1.epochToDateTime(instance?.auditDetails?.lastModifiedTime), instance?.assignes?.length > 0 ? instance.assignes[0]?.name : 'NA', - t(`${instance.comment}`) + instance?.comment ? t(instance.comment) : '' ], showConnector: true }));The changes:
- Add null checks for nested properties to prevent runtime errors
- Remove template literal for comment translation as it's unnecessary and could cause issues with dynamic content
- Provide fallback values for missing data
✅ Verification successful
Based on the search results, I can now provide a final response:
The review comment's concerns about translation safety are valid, but the suggested changes need revision.
The codebase shows consistent patterns of handling application status translations:
- The
applicationStatusis commonly used across the codebase as a workflow state identifier, and it's consistently translated using thet()function.- The current implementation follows the established pattern where status codes are directly translated without template literals.
- The comment translation should indeed be handled more safely, but the suggested implementation can be improved.
Here's the recommended improvement:
const steps = workflowData.ProcessInstances.map((instance, index) => ({ label: t(instance?.state?.applicationStatus || 'NA'), variant: 'completed', subElements: [Digit.Utils.microplanv1.epochToDateTime(instance?.auditDetails?.lastModifiedTime), instance?.assignes?.length > 0 ? instance.assignes[0]?.name : 'NA', - t(`${instance.comment}`) + instance?.comment ? t(instance.comment) : '-' ], showConnector: true }));Key changes:
- Keep direct translation of
applicationStatusas it's a standard workflow state identifier- Add null check for state access
- Improve comment translation with proper null check and a meaningful fallback value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the translation keys exist in the translation files echo "Searching for translation files and keys..." fd -e json . | xargs rg "applicationStatus|comment" -A 5Length of output: 155
Script:
#!/bin/bash # Let's try a different approach to find translation files and their content # First, let's check the directory structure to locate translation files fd "localization|i18n|translations" --type d # Then, let's look for any JSON files that might contain translations fd -e json . -x echo "=== {} ===" -x cat {} # Additionally, let's check how applicationStatus is used in the codebase rg "applicationStatus" -B 2 -A 2 # Let's also check for similar timeline components to understand the pattern ast-grep --pattern 'label: t($_)'Length of output: 42129
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/SearchJurisdiction.js (3)
7-7: LGTM: Component signature update is well-implemented.The addition of the
styleprop with a default empty object follows React best practices and maintains backward compatibility.
54-54: LGTM: Style prop is correctly applied.The style prop is properly applied to the root div, allowing for flexible styling while maintaining the base "search-wrapper" class.
57-57: LGTM: Consistent styling improvements.The styling changes improve layout consistency with full-width labels and proper spacing. The previous syntax error with double commas has been fixed.
Also applies to: 69-70
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FacilityPopup.js (4)
3-3: LGTM: Import changes are appropriateThe SVG import addition aligns with its usage in the component for visual feedback.
187-192: LGTM: Improved row selection handlingThe refactored row selection logic is more efficient and cleaner. It properly handles both individual and bulk selection states while maintaining a single source of truth for selection state.
409-411: LGTM: Well-implemented toast notification handlingThe dynamic toast type determination is clean and handles all cases properly while maintaining good code readability.
356-356: 🧹 Nitpick (assertive)Use self-closing tag for SVG component
For better readability and consistency with React conventions, use self-closing tags for components without children.
- <SVG.DoneAll width={"1.5rem"} height={"1.5rem"} fill={"#C84C0E"}></SVG.DoneAll> + <SVG.DoneAll width={"1.5rem"} height={"1.5rem"} fill={"#C84C0E"} />Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 356-356: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
...h/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FacilityPopup.js
Show resolved
Hide resolved
...h/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FacilityPopup.js
Show resolved
Hide resolved
Choose the appropriate template for your PR:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
These updates enhance user experience through improved functionality and visual design.