Vertical stepper implemenetation #1441
Conversation
|
Warning Rate limit exceeded@ashish-egov has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce several modifications within the campaign manager and microplan modules of the application. Key updates include the addition of the 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: 33
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/package.jsonis excluded by!**/*.json
📒 Files selected for processing (9)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/downloadExcel.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Module.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/BulkUpload.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/XlsPreview.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/useGenerateIdCampaign.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/downloadExcel.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/downloadExcel.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Module.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/BulkUpload.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/XlsPreview.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/useGenerateIdCampaign.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/downloadExcel.js (1)
Pattern
**/*.js: check
📓 Learnings (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1)
Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#845 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js:460-461 Timestamp: 2024-06-12T08:18:44.708Z Learning: Error handling for Shapefile parsing in `Upload.js` is managed commonly and is handled elsewhere in the codebase, as clarified by the user.
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/BulkUpload.js
[error] 128-135: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 190-196: 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)
[error] 183-183: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
[error] 126-126: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 176-176: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 193-193: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/XlsPreview.js
[error] 9-9: Alternative text title element cannot be empty
For accessibility purposes, SVGs should have an alternative text, provided via title element. If the svg element has role="img", you should add the aria-label or aria-labelledby attribute.
(lint/a11y/noSvgWithoutTitle)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js
[error] 100-100: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/downloadExcel.js
[error] 18-18: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🔇 Additional comments (11)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/useGenerateIdCampaign.js (1)
Line range hint
1-24: Summary: Good implementation of thesourceparameterThe changes to add the
sourceparameter to theuseGenerateIdCampaignfunction are well-implemented and maintain backward compatibility. The new parameter allows for more flexibility in the API request criteria without altering the existing functionality.To ensure the changes are used correctly throughout the codebase, consider the following:
Run the following script to check for any usage of
useGenerateIdCampaignthat might need updating:Review the results to ensure that any calls to
useGenerateIdCampaignare updated if they need to use the newsourceparameter.✅ Verification successful
All usages of
useGenerateIdCampaigninclude the newsourceparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of useGenerateIdCampaign and check if they need to be updated with the new 'source' parameter. # Search for useGenerateIdCampaign usage rg --type js 'useGenerateIdCampaign\s*\(' -A 5Length of output: 885
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/downloadExcel.js (1)
1-13: LGTM: Import and documentation are well-structured.The import statement is correct, and the comment block provides a clear explanation of the function's purpose and usage. This is good practice for maintaining readable and maintainable code.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/XlsPreview.js (2)
1-5: LGTM: Imports and constants are well-organized.The necessary imports are present, and the use of a constant for the primary color promotes consistency throughout the component.
1-69: Overall assessment: Well-implemented component with room for minor improvementsThe XlsPreview component is a solid implementation for previewing Excel files. It demonstrates good use of React practices, internationalization, and third-party libraries. The suggestions provided earlier will further enhance its accessibility, error handling, and maintainability.
Key strengths:
- Clear component structure
- Proper use of internationalization
- Effective integration of DocViewer for file preview
Areas for improvement:
- SVG accessibility
- Error handling for missing file prop
- Extraction of inline styles
Implementing these suggestions will result in a more robust and maintainable component.
🧰 Tools
🪛 Biome
[error] 9-9: Alternative text title element cannot be empty
For accessibility purposes, SVGs should have an alternative text, provided via title element. If the svg element has role="img", you should add the aria-label or aria-labelledby attribute.
(lint/a11y/noSvgWithoutTitle)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Module.js (3)
12-12: LGTM: Import statement for UploadDataCustomThe import statement for the new
UploadDataCustomcomponent follows best practices and naming conventions.
59-59: LGTM: UploadDataCustom added to componentsToRegisterThe
UploadDataCustomcomponent is correctly added to thecomponentsToRegisterobject, following the existing pattern.
12-12: Verify the usage of UploadDataCustom componentThe
UploadDataCustomcomponent has been imported and registered correctly. However, it's important to ensure that this new component is being used appropriately within the application.Could you please provide information on where and how the
UploadDataCustomcomponent is being used? This will help ensure that the addition is serving its intended purpose.To assist in verifying the component's usage, you can run the following script:
Also applies to: 59-59
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js (2)
101-102: Verify consistency betweenkeyandpopulators.nameIn this step,
keyis set to"uploadData", butpopulators.nameis set to"hypothesis".Typically,
keyandpopulators.nameshould be consistent to ensure proper data mapping and handling.Please confirm whether this discrepancy is intentional. If not, consider updating
populators.nameto"uploadData"to match thekey.Also applies to: 117-119
110-111: Potential redundancy betweentypeandtypesincustomPropsWithin
customProps, bothtypeandtypesare defined:
type: "facilityWithBoundary"types: ["boundary", "facilityWithBoundary"]Unless both properties are required for different purposes, having both may be redundant and could cause confusion.
Please verify if both
typeandtypesare necessary. If only one is needed, consider removing the redundant property to simplify the configuration.health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/BulkUpload.js (1)
155-155: Verify correct file is being downloadedIn the
handleFileDownloadfunction call,fileUrlis being passed, which may not correspond to the current file in the loop. Ensure that the correct file is being downloaded for each item.health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1)
817-837:⚠️ Potential issuePrevent potential infinite re-rendering
The
useEffecthook depends onerrorsType, and inside the effect, you update state variables that could triggererrorsTypechanges, potentially causing infinite re-renders.Ensure that state updates within
useEffectdo not cause unintended loops:817 useEffect(() => { const fetchData = async () => { if (!errorsType[type] && uploadedFile?.length > 0 && !isSuccess) { // existing code... + // Add a condition to prevent unnecessary state updates + if (!isError) { setIsError(true); + } // existing code... } }; fetchData(); }, [errorsType]);Likely invalid or redundant comment.
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/useGenerateIdCampaign.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/useGenerateIdCampaign.js
Outdated
Show resolved
Hide resolved
...micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/downloadExcel.js
Outdated
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/downloadExcel.js
Outdated
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/downloadExcel.js
Outdated
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| } catch (error) { | ||
| console.log(error); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use console.error instead of console.log for error logging
Using console.error provides better visibility for errors in the console and differentiates error messages from regular logs.
Apply this change to improve error logging:
759
- console.log(error);
+ console.error("Error while validating Excel file:", error);📝 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.
| console.log(error); | |
| console.error("Error while validating Excel file:", error); |
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
…lan/src/hooks/useGenerateIdCampaign.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…lan/src/utils/downloadExcel.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| @@ -94,6 +94,22 @@ const SetupMicroplan = ({ hierarchyType, hierarchyData }) => { | |||
| setMicroplanConfig(MicroplanConfig(params, null, isSubmitting, null, hierarchyData)); | |||
| }, [totalFormData, isSubmitting]); | |||
|
|
|||
| useEffect(() => { | |||
| const handleCheckingEvent = () => { | |||
| console.log("Checking event triggered"); | |||
…/egovernments/DIGIT-Frontend into vertical-stepper-implemenetation
There was a problem hiding this comment.
Actionable comments posted: 16
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (3)
micro-ui/web/micro-ui-internals/packages/react-components/package.jsonis excluded by!**/*.jsonmicro-ui/web/workbench/package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonmicro-ui/web/workbench/package.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Module.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js (7 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/useGenerateIdCampaign.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (4 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/downloadExcel.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Module.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/useGenerateIdCampaign.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/downloadExcel.js (1)
Pattern
**/*.js: check
📓 Learnings (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/useGenerateIdCampaign.js (1)
Learnt from: ashish-egov PR: egovernments/DIGIT-Frontend#1441 File: health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/useGenerateIdCampaign.js:1-1 Timestamp: 2024-09-30T05:45:14.549Z Learning: In the `useGenerateIdCampaign` function, auto assignment of default parameter values is needed in parameter destructuring.
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
[error] 138-138: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 188-188: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 205-205: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js
[error] 156-156: This property value named key is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named key(lint/suspicious/noDuplicateObjectKeys)
[error] 181-184: This property value named populators is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named populators(lint/suspicious/noDuplicateObjectKeys)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js
[error] 100-100: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
🔇 Additional comments (5)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/useGenerateIdCampaign.js (1)
Line range hint
1-26: Overall, the changes look good with minor suggestions for improvement.The implementation of the
sourceparameter is correct and maintains backward compatibility. The suggestions provided are mainly for consistency with coding preferences and improved readability. Great job on extending the functionality of theuseGenerateIdCampaignhook!health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Module.js (2)
13-13: LGTM: Import statement for UploadDataCustom is correct.The import statement for the new
UploadDataCustomcomponent is syntactically correct and follows the expected project structure.
13-13: Summary: UploadDataCustom component successfully integrated.The changes in this file successfully integrate the new
UploadDataCustomcomponent into the Microplan module. The component is properly imported and registered, allowing it to be used within the Digit framework. These changes enhance the module's functionality by introducing custom upload capabilities.To ensure a smooth integration:
- Verify that the
UploadDataCustomcomponent is fully implemented and tested.- Update any relevant documentation to reflect the new upload functionality.
- Consider adding unit tests for the new component if not already present.
Also applies to: 63-63
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1)
17-17: LGTM: New import statement added.The new import statement for additional UI components from "@egovernments/digit-ui-components" is correctly formatted and likely supports the updated component functionality.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js (1)
97-125: Previous comment about 'stepCount' and 'key' inconsistencies still appliesThe issues regarding inconsistent
stepCountandkeyvalues, as mentioned in the previous review comment, are still present. Please refer to the prior feedback to resolve these inconsistencies.
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/useGenerateIdCampaign.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/useGenerateIdCampaign.js
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Module.js
Show resolved
Hide resolved
| useEffect(() => { | ||
| const handleCheckingEvent = () => { | ||
| console.log("Checking event triggered"); | ||
| const newKey = parseInt(new URLSearchParams(window.location.search).get("key")) || 1; | ||
| setCurrentKey(newKey); | ||
| }; | ||
|
|
||
| window.addEventListener("checking", handleCheckingEvent); | ||
|
|
||
| return () => { | ||
| window.removeEventListener("checking", handleCheckingEvent); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the custom event handling
The new effect adds a custom "checking" event listener to update the currentKey state based on URL changes. While this approach works, there are a few points to consider:
-
The use of a custom "checking" event is unconventional. Consider using a more standard approach, such as the
useLocationhook fromreact-router-domto listen for URL changes. -
Remove the
console.logstatement before deploying to production. -
When using
parseInt, it's best practice to provide a radix parameter to ensure consistent behavior across different environments.
Consider refactoring the effect as follows:
import { useLocation } from 'react-router-dom';
// ... other imports and code
const location = useLocation();
useEffect(() => {
const searchParams = new URLSearchParams(location.search);
const newKey = Number.parseInt(searchParams.get("key"), 10) || 1;
setCurrentKey(newKey);
}, [location]);This approach eliminates the need for a custom event, removes the console.log, and uses Number.parseInt with a radix for better consistency.
🧰 Tools
🪛 Biome
[error] 100-100: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js
Show resolved
Hide resolved
| { schemaCode: "HCM-ADMIN-CONSOLE.adminSchema" } | ||
| ); | ||
|
|
||
| console.log(Schemas, " ssssssssssssssssssssssssssssss") |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid using console.log statements in production code
The console.log statement on line 53 appears to be leftover from debugging. It's recommended to remove it to clean up the code.
Apply this diff to remove the console.log statement:
- console.log(Schemas, " ssssssssssssssssssssssssssssss")📝 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.
| console.log(Schemas, " ssssssssssssssssssssssssssssss") |
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Outdated
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range comments (3)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js (3)
Line range hint
54-97: LGTM! Improved variable declaration and error handling.The changes in the
CreateResourcefunction look good. ThecampaignObjectis now correctly defined without redeclaration, and the error handling has been improved to provide more specific error messages.A minor suggestion for further improvement:
Consider using optional chaining for nested object properties to make the code more robust. For example:
if (campaignRes?.CampaignDetails?.id && planRes?.PlanConfiguration?.[0]?.id) { // ... }
Line range hint
160-179: LGTM! Improved flow control and error handling.The changes in the CAMPAIGN_DETAILS and MICROPLAN_DETAILS cases of the
createUpdatePlanProjectfunction look good. The flow control has been improved, and error handling with user feedback has been added usingsetShowToast.A minor suggestion for improvement:
Consider adding more specific error messages for different failure scenarios. For example:
if (!isResourceNameValid) { setShowToast({ key: "error", label: "ERROR_MICROPLAN_NAME_ALREADY_EXISTS" }); return; } const isResourceCreated = await CreateResource(req); if (!isResourceCreated) { setShowToast({ key: "error", label: "ERROR_CREATING_MICROPLAN" }); return; }This will provide more informative feedback to the user about what went wrong.
🧰 Tools
🪛 Biome
[error] 161-161: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
Line range hint
179-205: LGTM with suggestions for improvement.The changes in the BOUNDARY case of the
createUpdatePlanProjectfunction look good overall. The plan update logic has been improved with proper error handling and user feedback.However, there are a couple of points that need attention:
The
startDateis hardcoded to 100 days from now. This might not be the intended behavior and could lead to unexpected results. Consider making this configurable or using a value from thetotalFormData.The error handling for the plan update could be more specific. Instead of a generic "ERR_BOUNDARY_UPDATE" message, consider providing more detailed error information.
Suggested improvements:
const updatedCampaignObject = { ...campaignObject, boundaries: totalFormData?.BOUNDARY?.boundarySelection?.selectedData, startDate: totalFormData?.CAMPAIGN_DETAILS?.startDate || Math.floor(new Date().setDate(new Date().getDate() + 100)) }; const planRes = await updatePlan(updatedCampaignObject); if (planRes?.CampaignDetails?.id) { setCurrentKey((prev) => prev + 1); setCurrentStep((prev) => prev + 1); return { triggeredFrom }; } else { setShowToast({ key: "error", label: `ERR_BOUNDARY_UPDATE: ${planRes?.error || 'Unknown error'}` }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js (7 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js (6 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
[error] 134-134: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 182-182: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 199-199: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js
[error] 100-100: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
[error] 249-254: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.(lint/style/noVar)
🔇 Additional comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js (1)
151-177: InconsistentstepCountandkeyvalues may affect navigation flowThe
stepCountandkeyvalues in the steps starting at lines 151 and 179 are inconsistent:
- The
stepCount"5" is used for both steps at lines 151 and 179.- The
keyvalues are not sequential: "6" and "7" for the samestepCount.- Both steps have the
name"UPLOADDATA".This inconsistency can lead to issues in the navigation flow and rendering of the steps in the UI. Please review and ensure that
stepCountandkeyvalues are sequential and unique.Also applies to: 179-199
.../micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js
Show resolved
Hide resolved
.../micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js
Show resolved
Hide resolved
.../micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js
Show resolved
Hide resolved
...cro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js
Outdated
Show resolved
Hide resolved
...cro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js
Outdated
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
…lan/src/components/UploadDataCustom.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
...micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/downloadExcel.js
Outdated
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/XlsPreview.js
Outdated
Show resolved
Hide resolved
...cro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js
Outdated
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/downloadExcel.js
Outdated
Show resolved
Hide resolved
…/egovernments/DIGIT-Frontend into vertical-stepper-implemenetation
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range comments (5)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/UploadData.js (3)
Line range hint
82-86: Ensure proper dependency array for useEffect.The useEffect hook correctly updates
keyandcurrentStepbased oncurrentKey. However, the dependency array is missingsetKeyandsetCurrentStep. While React guarantees the stability of setState functions, it's a good practice to include all variables used within the effect in the dependency array.useEffect(() => { setKey(currentKey); setCurrentStep(currentKey - baseKey + 1); }, [currentKey, setKey, setCurrentStep, baseKey]);This change ensures that the effect runs when any of its dependencies change, preventing potential stale closure issues.
Line range hint
240-244: Good addition of inactive facility filtering.The new
filterByUpdateFlagfunction is a good addition that filters out properties marked for update. This is applied to the headers for boundary, facility, and user data. This change improves the data processing by ensuring only relevant fields are considered.However, consider adding a comment explaining the purpose of this filtering, as it might not be immediately clear to other developers why certain properties are being excluded.
// Filter out properties marked for update to prevent unintended modifications const filterByUpdateFlag = (schemaProperties) => { return Object.keys(schemaProperties).filter( (key) => schemaProperties[key].isUpdate !== true ); };This addition will help maintain code clarity and assist future developers in understanding the purpose of this filtering.
Line range hint
1-1193: Overall, good improvements with some refactoring opportunities.The changes to the
UploadDatacomponent generally improve its functionality and structure. Key improvements include better handling of different data types, enhanced UI with the addition of aSteppercomponent, and more robust error handling.However, there are several opportunities for further refactoring to enhance readability and maintainability:
- Consider combining related state variables into objects.
- Ensure all useEffect hooks have proper dependency arrays.
- Simplify complex conditional logic using lookup objects or separate functions.
- Address the empty catch block in the error handling section.
- Extract complex JSX structures into separate components or functions.
These refactoring suggestions will make the code more maintainable and easier to understand for future developers working on this component.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/BulkUpload.js (2)
Line range hint
31-36: Fix the destructuring assignment in thefetchfunctionThe destructuring assignment in the
fetchfunction may not correctly retrieve thefileStoreIdsfrom the response. The line:const { data: { fileStoreIds: fileUrl } = {} } = await Digit.UploadServices.Filefetch([fileData?.[0]?.filestoreId], tenantId);is attempting to rename
fileStoreIdstofileUrl, which can lead tofileUrlbeingundefinedand cause issues in the subsequent code.Please adjust the code to correctly extract the file URL from the response. For example:
const response = await Digit.UploadServices.Filefetch([fileData?.[0]?.filestoreId], tenantId); const fileStoreIds = response?.data?.fileStoreIds; const temp = fileData?.map((i) => ({ ...i, url: fileStoreIds?.[i.filestoreId]?.url, })); setFileUrl(temp?.[0]);
Line range hint
69-72: Reinstate Excel file validation in thehandleChangefunctionIn the
handleChangefunction, the call tovalidateExcelis commented out:// await validateExcel(newFiles[0]);As a result, Excel files are no longer being validated before submission. This might lead to invalid or incorrect files being processed, which could cause downstream issues.
Please consider enabling the Excel validation or ensure that file validation is handled appropriately elsewhere. If the validation is intentionally skipped, please confirm that it's safe to proceed without it.
To re-enable validation, uncomment the line:
await validateExcel(newFiles[0]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (8)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (5 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/BulkUpload.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/UploadData.js (9 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/index.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/index.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js (5 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (4 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/BulkUpload.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/UploadData.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/index.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/index.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/SetupMicroplanConfig.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
[error] 132-132: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 180-180: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 197-197: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js
[error] 99-99: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
🔇 Additional comments (10)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/index.js (1)
4-4: LGTM: Default export updated to includedownloadExcelWithCustomNameThe change to include
downloadExcelWithCustomNamein the default export is a good improvement. It consolidates the exports and makes it easier for other modules to import and use this function.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/index.js (2)
49-49: LGTM: Minor formatting improvementThe added space before the closing curly brace in the
samplemethod improves code readability by following a consistent formatting style.
54-56: Clarify the purpose of the newcampaignproperty inUtilsThe addition of the
campaignproperty to theUtilsobject, which spreads the properties of the existingutilsobject, raises a few concerns:
- Potential naming conflicts: If there are overlapping property names between
utilsand other properties ofUtils, this could lead to unexpected behavior.- Redundancy: The
workbenchproperty already includes the same spread ofutils. What's the specific need for a separatecampaignproperty?- Scope and intention: It's unclear why campaign-related utilities are being added to the general
Utilsobject rather than being kept separate in thecampaignobject defined earlier in the file.Could you please clarify the reasoning behind this addition and confirm that it doesn't introduce any conflicts or redundancies?
To check for potential naming conflicts, you can run the following script:
This script will help identify any overlapping property names between the
Utilsobject and the importedutilsobject.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (4)
36-36: LGTM: Import statement formatting improvedThe formatting change in the import statement for
CreateChecklistimproves code readability and consistency with other import statements in the file. This is a good practice for maintaining clean and uniform code.
Line range hint
1-185: Overall assessment: Changes improve code quality and introduce new functionalityThe modifications in this file primarily focus on:
- Improving code formatting and consistency
- Introducing a new XlsPreview component
These changes enhance the module's functionality and maintain good coding practices. The new XlsPreview component adds Excel preview capabilities, which could be a valuable feature for the campaign manager module.
To ensure the changes are fully integrated:
- Verify that the XlsPreview component is properly implemented and tested in its source file.
- Check if any documentation needs to be updated to reflect the new XlsPreview functionality.
- Confirm that the XlsPreview component is used correctly in other parts of the codebase where Excel preview functionality is required.
119-119: Component registration updates look good
- The formatting change for
CreateQuestionimproves consistency with other entries in thecomponentsToRegisterobject.- The addition of
XlsPreviewtocomponentsToRegisteris consistent with the new import and ensures the component is properly registered for use within the module.These changes enhance code readability and correctly integrate the new XlsPreview component.
Let's verify the XlsPreview component's registration:
#!/bin/bash # Search for XlsPreview in the componentsToRegister object rg --type js 'componentsToRegister\s*=\s*\{[\s\S]*XlsPreview' health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.jsAlso applies to: 129-129
48-48: New XlsPreview component importedThe addition of the
XlsPreviewimport suggests a new feature for Excel preview functionality. This is a good addition that likely enhances the module's capabilities.To ensure proper integration, let's verify the usage of this new component:
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/BulkUpload.js (1)
27-27: Verify thatXlsPreviewis correctly registered in the Component RegistryThe dynamic retrieval of
XlsPreviewusingDigit?.ComponentRegistryService?.getComponent("XlsPreview")assumes thatXlsPreviewhas been registered in the Component Registry. Please verify thatXlsPreviewis properly registered to avoid runtime errors.Run the following script to confirm the registration of
XlsPreview:health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (2)
97-109: Consider refactoring the custom event handlingAs previously noted, the use of the custom "checking" event listener is unconventional. Consider using the
useLocationhook fromreact-router-domto listen for URL changes. This approach promotes better integration with React Router and avoids the need for custom events.🧰 Tools
🪛 Biome
[error] 99-99: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
135-135: 🧹 Nitpick (assertive)Unnecessary parentheses in
setShowToastcallThe
setShowToastfunction is invoked with extra parentheses around the object argument. This is unnecessary and may cause confusion. Consider removing the outer parentheses for clarity.Apply this diff to fix the issue:
-setShowToast(({ key: "error", label: error?.message ? error.message : t("FAILED_TO_UPDATE_RESOURCE") })) +setShowToast({ key: "error", label: error?.message ? error.message : t("FAILED_TO_UPDATE_RESOURCE") })Likely invalid or redundant comment.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/index.js
Show resolved
Hide resolved
...cro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/UploadData.js
Outdated
Show resolved
Hide resolved
...cro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/UploadData.js
Outdated
Show resolved
Hide resolved
...cro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/UploadData.js
Outdated
Show resolved
Hide resolved
...cro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/UploadData.js
Outdated
Show resolved
Hide resolved
...cro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/BulkUpload.js
Outdated
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
Summary by CodeRabbit
Release Notes
New Features
UploadDataCustomcomponent for handling data file uploads with validation and feedback mechanisms.XlsPreviewcomponent for displaying Excel file previews in a user-friendly popup.MicroplanConfig, enhancing user workflow.Improvements
SetupMicroplanfor dynamic URL parameter handling.Bug Fixes