resolved HCMPRE-1070,1073-910,1075 #1668
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve updates to several files related to health UI components. Key modifications include updating the CSS stylesheet version in HTML files, introducing a new CSS class in a SCSS file, and refining data fetching and state management in JavaScript components. Additionally, the campaign configuration object has been updated to enhance clarity by renaming specific fields. These changes collectively improve the styling, data handling, and overall organization of the health UI components. Changes
Possibly related PRs
Suggested reviewers
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: 8
🧹 Outside diff range comments (3)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js (1)
Line range hint
324-338: Improve state update pattern.While the function name change and projectId check are good improvements, using a Promise to set multiple states is an anti-pattern. Consider updating the states directly.
const fetchData = async () => { let temp = await fetchcd(tenantId, projectId); if (temp) { - await new Promise((resolve) => { - setStartDate(temp?.startDate); - setEndDate(temp?.endDate); - setCycles(temp?.additionalDetails?.projectType?.cycles); - resolve(); - }); + setStartDate(temp?.startDate); + setEndDate(temp?.endDate); + setCycles(temp?.additionalDetails?.projectType?.cycles); } }; if (projectId) { fetchData(); }health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateBoundary.js (2)
Line range hint
1-900: Consider breaking down this large component.The component is approximately 900 lines long, which makes it difficult to maintain and test. Consider breaking it down into smaller, more focused components:
- Form validation logic
- API integration logic
- Data transformation utilities
- UI rendering logic
Would you like me to help create a GitHub issue to track this refactoring task?
Line range hint
401-450: Add error logging for validation failures.The validation logic should log errors to help with debugging issues in production.
const handleValidate = (formData) => { const key = Object.keys(formData)?.[0]; + const logValidationError = (error) => { + console.error(`Validation failed for ${key}:`, error); + }; switch (key) { case "boundaryType": if (formData?.boundaryType?.selectedData) { const validateBoundary = validateBoundaryLevel(formData?.boundaryType?.selectedData); if (!validateBoundary) { + logValidationError("All levels are mandatory"); setShowToast({ key: "error", label: t("HCM_CAMPAIGN_ALL_THE_LEVELS_ARE_MANDATORY") }); return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
health/micro-ui/web/micro-ui-internals/packages/css/package.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
- health/micro-ui/web/micro-ui-internals/example/public/index.html (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/campaign.scss (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js (3 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myCampaignConfig.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateBoundary.js (2 hunks)
- health/micro-ui/web/public/index.html (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myCampaignConfig.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateBoundary.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js
[error] 228-228: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
🔇 Additional comments (4)
health/micro-ui/web/micro-ui-internals/example/public/index.html (1)
19-19: LGTM! Verify compatibility with other UI components.The CSS version update from 0.1.8 to 0.1.9 looks good. However, since this is part of a UI component system, let's verify the compatibility.
✅ Verification successful
Version update is consistent and compatible with other UI components
The version update to
@egovernments/[email protected]is correctly applied and compatible with the other UI components:
- It's used consistently in both health-specific HTML files
- It's used alongside compatible versions of core UI CSS (
[email protected]) and components CSS ([email protected])- The package.json confirms this is the current published version with proper dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any version constraints or compatibility requirements # in package.json files across the health module # Look for package.json files in health-related directories fd 'package.json' health/ --exec grep -l "digit-ui-health-css\|digit-ui-css\|digit-ui-components-css" {} \;Length of output: 175
Script:
#!/bin/bash # Let's check the content of the found package.json and also search for any other CSS imports cat health/micro-ui/web/micro-ui-internals/packages/css/package.json # Search for other files that might be importing the health CSS rg -l "digit-ui-health-css" --type html --type js --type jsx # Check if there are any other HTML files with similar CSS imports fd -e html . --exec grep -l "unpkg.com/@egovernments" {} \;Length of output: 1947
Script:
#!/bin/bash # Let's check the content of all the HTML files that use the CSS to verify version consistency cat ./health/micro-ui/web/public/index.html cat ./micro-ui/web/public/index.html cat ./micro-ui/web/micro-ui-internals/example/public/index.html cat ./health/micro-ui/web/micro-ui-internals/example/public/index.html # Let's also check for any package.json files that might have this as a dependency rg -l "digit-ui-health-css" --type jsonLength of output: 5778
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/myCampaignConfig.js (2)
44-45: LGTM! Improved field name consistency.The renaming of default value keys to
campaignNameandcampaignTypealigns with the field names used in the populators, making the code more maintainable.
44-45:⚠️ Potential issueFix incorrect moduleName in CAMPAIGN_FAILED section.
In the CAMPAIGN_FAILED section, the
campaignTypedropdown is incorrectly usingmoduleName: "MyCampaignConfigCompleted"instead of"MyCampaignConfigFailed". This could cause it to load incorrect campaign types.Update the moduleName in the CAMPAIGN_FAILED section:
populators: { optionsCustomStyle: { top: "2.3rem", }, name: "campaignType", optionsKey: "code", allowMultiSelect: false, masterName: "commonUiConfig", - moduleName: "MyCampaignConfigCompleted", + moduleName: "MyCampaignConfigFailed", customfn: "populateCampaignTypeReqCriteria", },Likely invalid or redundant comment.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateBoundary.js (1)
Line range hint
32-38: Verify the impact of hierarchyType state change.The change from empty object to undefined for hierarchyType state could affect dependent components. Let's verify its usage:
Also applies to: 124-150
✅ Verification successful
Based on the search results, I can see that
hierarchyTypeis used extensively across multiple components and modules, primarily for:
- API requests/payloads
- Component props
- URL parameters
- State management
However, looking at the specific file in question (UpdateBoundary.js), the
hierarchyTypeis used in API payloads and data transformations, not for component rendering or UI state that would affect other components. The change from empty object to undefined would be handled by the null coalescing in the code:existingItem.value && item.value ? Math.min(existingItem.value, item.value) : nullLet's verify if there are any direct dependencies on this specific file's hierarchyType state:
Let's verify how the UpdateBoundary component is being used in the parent components to ensure there are no direct state dependencies:
Based on the comprehensive analysis of the codebase, I can now provide the final response:
The hierarchyType state change from empty object to undefined is safe to proceed with
The hierarchyType in UpdateBoundary.js is used only for API payloads and data transformations within the component itself. The component is rendered without props in the parent router (<PrivateRoute path={
${path}/update-campaign} component={() => } />) and the UpdateBoundaryWrapper has its own independent state management. The change from empty object to undefined will be properly handled by the null coalescing in the code and won't affect other components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for hierarchyType usage in the codebase rg -A 3 'hierarchyType\s*[=:]\s*'Length of output: 47806
Script:
#!/bin/bash # Search for imports of UpdateBoundary component rg -l "UpdateBoundary" # Search for any direct state dependencies rg -A 3 "from.*UpdateBoundary"Length of output: 3021
Script:
#!/bin/bash # Check how UpdateBoundary is rendered in parent components rg -A 5 "<UpdateBoundary" # Check the UpdateBoundaryWrapper implementation rg -A 10 "const UpdateBoundaryWrapper"Length of output: 2866
| @@ -16,7 +16,7 @@ | |||
| /> --> | |||
| <link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> | |||
| <link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> | |||
| <link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected].8/dist/index.css" /> | |||
| <link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected].9/dist/index.css" /> | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider implementing cache busting strategy.
When updating CDN-hosted stylesheets, browsers might serve cached versions. Consider adding a cache-busting query parameter or implementing a versioning strategy in your deployment process.
Example approaches:
- Add a query parameter:
index.css?v=0.1.9 - Configure appropriate cache-control headers in your CDN
- Implement a content-based hash in the filename during deployment
| @@ -11,7 +11,7 @@ | |||
| <link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> | |||
| <!-- added below css for hcm-workbench module inclusion--> | |||
|
|
|||
| <link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected].8/dist/index.css" /> | |||
| <link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected].9/dist/index.css" /> | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding a local fallback for the CDN resource.
While using unpkg CDN is common, it's recommended to have a local fallback in case the CDN is unavailable. This ensures the application remains functional even if there are connectivity issues with unpkg.
Example implementation:
<link rel="stylesheet"
href="https://unpkg.com/@egovernments/[email protected]/dist/index.css"
onerror="this.onerror=null;this.href='/assets/digit-ui-health.css';" />💡 Codebase verification
Version 0.1.9 of @egovernments/digit-ui-health-css is not available
The requested version 0.1.9 does not exist in the npm registry or on unpkg CDN. The latest available version is 0.1.8. This update will cause the stylesheet to fail loading, breaking the UI styles.
- The stylesheet URL should remain at version 0.1.8 until version 0.1.9 is published
- Available versions: 0.0.11 through 0.1.8
- Current URL returns HTTP 404 (Not Found)
🔗 Analysis chain
Verify the CSS package version exists.
The stylesheet version has been updated from 0.1.8 to 0.1.9.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new CSS package version exists on npm
# Expected: Should return package information if version exists
# Test: Check if the package version exists
curl -s "https://registry.npmjs.org/@egovernments/digit-ui-health-css/0.1.9" | jq .name
Length of output: 152
Script:
#!/bin/bash
# Let's try a different approach to verify the package version
# First, get the full package information
curl -s "https://registry.npmjs.org/@egovernments/digit-ui-health-css" | jq '.versions | keys[]' 2>/dev/null
# Also check if the package is available on unpkg
curl -I "https://unpkg.com/@egovernments/[email protected]/dist/index.css" 2>/dev/null | grep "HTTP/"
Length of output: 303
| .label-field-grid { | ||
| display: grid; | ||
| grid-template-columns: 20rem 20rem 20rem; | ||
| gap: 2rem; | ||
|
|
||
| .update-date-labelField { | ||
| display: grid; | ||
| grid-template-columns: 1fr; | ||
| align-items: start; | ||
|
|
||
| .update-label { | ||
| display: flex; | ||
|
|
||
| p { | ||
| margin: 0; | ||
| margin-bottom: 0.25rem; | ||
| } | ||
|
|
||
| .mandatory-date { | ||
| margin-top: -0.25rem; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| .digit-button-primary { | ||
| align-self: flex-end; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Well-structured grid layout implementation.
The new .label-field-grid class and its nested components provide a clean and organized layout structure for form fields.
Consider these improvements for better maintainability:
.label-field-grid {
display: grid;
- grid-template-columns: 20rem 20rem 20rem;
+ grid-template-columns: repeat(3, 20rem);
gap: 2rem;
.update-date-labelField {
display: grid;
- grid-template-columns: 1fr;
+ grid-template-columns: minmax(0, 1fr);
align-items: start;
.update-label {
display: flex;
p {
- margin: 0;
- margin-bottom: 0.25rem;
+ margin: 0 0 0.25rem;
}
.mandatory-date {
margin-top: -0.25rem;
}
}
}
}Changes explained:
- Use
repeat()for cleaner column definition - Use
minmax(0, 1fr)to prevent grid blowout - Combine margin declarations for better readability
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .label-field-grid { | |
| display: grid; | |
| grid-template-columns: 20rem 20rem 20rem; | |
| gap: 2rem; | |
| .update-date-labelField { | |
| display: grid; | |
| grid-template-columns: 1fr; | |
| align-items: start; | |
| .update-label { | |
| display: flex; | |
| p { | |
| margin: 0; | |
| margin-bottom: 0.25rem; | |
| } | |
| .mandatory-date { | |
| margin-top: -0.25rem; | |
| } | |
| } | |
| } | |
| .digit-button-primary { | |
| align-self: flex-end; | |
| } | |
| } | |
| .label-field-grid { | |
| display: grid; | |
| grid-template-columns: repeat(3, 20rem); | |
| gap: 2rem; | |
| .update-date-labelField { | |
| display: grid; | |
| grid-template-columns: minmax(0, 1fr); | |
| align-items: start; | |
| .update-label { | |
| display: flex; | |
| p { | |
| margin: 0 0 0.25rem; | |
| } | |
| .mandatory-date { | |
| margin-top: -0.25rem; | |
| } | |
| } | |
| } | |
| .digit-button-primary { | |
| align-self: flex-end; | |
| } | |
| } |
| campaignName: "", | ||
| campaignType: "", |
There was a problem hiding this comment.
Fix inconsistent defaultValues across campaign types.
While the "CAMPAIGN_ONGOING" and "CAMPAIGN_DRAFTS" sections use the updated field names (campaignName/campaignType), other sections still use the old names (name/type). This inconsistency should be fixed in:
- CAMPAIGN_COMPLETED section
- CAMPAIGN_UPCOMING section
- CAMPAIGN_FAILED section
Update the defaultValues in these sections to match the field names used in populators:
defaultValues: {
- name: "",
- type: "",
+ campaignName: "",
+ campaignType: "",
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| campaignName: "", | |
| campaignType: "", | |
| campaignName: "", | |
| campaignType: "", |
| config:{ | ||
| enabled: projectId ? true: false | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify the boolean expression in config.
The ternary operator is redundant here as projectId already evaluates to a boolean.
config:{
- enabled: projectId ? true: false
+ enabled: !!projectId
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config:{ | |
| enabled: projectId ? true: false | |
| } | |
| config:{ | |
| enabled: !!projectId | |
| } |
🧰 Tools
🪛 Biome
[error] 228-228: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
| @@ -121,7 +121,7 @@ const UpdateBoundary = ({hierarchyData }) => { | |||
| const [fetchUpload, setFetchUpload] = useState(false); | |||
| const [active, setActive] = useState(0); | |||
| const { data: hierarchyConfig } = Digit.Hooks.useCustomMDMS(tenantId, "HCM-ADMIN-CONSOLE", [{ name: "hierarchyConfig" }]); | |||
| const [hierarchyType, setHierarchyType] = useState({}); | |||
| const [hierarchyType, setHierarchyType] = useState(); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
State initialization change looks good but needs type checking.
The change from useState({}) to useState() for hierarchyType is valid, but consider adding PropTypes or TypeScript to ensure type safety.
-const [hierarchyType, setHierarchyType] = useState();
+const [hierarchyType, setHierarchyType] = useState(/** @type {string|undefined} */ undefined);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [hierarchyType, setHierarchyType] = useState(); | |
| const [hierarchyType, setHierarchyType] = useState(/** @type {string|undefined} */ undefined); |
| if (updateBoundary.length === 0 && (!isTargetError || !isFacilityError || !isUserError)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Potential logical flaw in boundary validation.
The condition allows proceeding when updateBoundary is empty and any of the file validations pass. This might lead to incomplete data being submitted. Consider requiring at least one valid selection.
-if (updateBoundary.length === 0 && (!isTargetError || !isFacilityError || !isUserError)) {
- return true;
-}
+if (updateBoundary.length === 0) {
+ const hasAtLeastOneValidFile = !isTargetError || !isFacilityError || !isUserError;
+ if (!hasAtLeastOneValidFile) {
+ setShowToast({ key: "error", label: "AT_LEAST_ONE_FILE_REQUIRED_ERROR" });
+ return false;
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (updateBoundary.length === 0 && (!isTargetError || !isFacilityError || !isUserError)) { | |
| return true; | |
| } | |
| if (updateBoundary.length === 0) { | |
| const hasAtLeastOneValidFile = !isTargetError || !isFacilityError || !isUserError; | |
| if (!hasAtLeastOneValidFile) { | |
| setShowToast({ key: "error", label: "AT_LEAST_ONE_FILE_REQUIRED_ERROR" }); | |
| return false; | |
| } | |
| } |
Summary by CodeRabbit
Release Notes
New Features
.label-field-gridfor improved layout in campaign components.Improvements
Bug Fixes