Feature/hcmpre 1903 #2183
Feature/hcmpre 1903 #2183
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the campaign management module, focusing on improving data filtering, loading state management, and component configuration. Changes span multiple files in the campaign manager, including Changes
Possibly related PRs
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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: 12
🔭 Outside diff range comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundaries.js (1)
Line range hint
16-24: Consider implementing state management library.The component uses multiple useState hooks and complex state interactions. Consider using a state management library (e.g., Redux, Zustand) to:
- Centralize state management
- Improve code maintainability
- Reduce prop drilling
- Make state changes more predictable
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js (1)
Line range hint
7-30: Consider destructuring props for better readability.Instead of accessing props through nested objects, consider destructuring them at the component level for cleaner code.
-const CampaignSelection = ({ onSelect, formData, formState, ...props }) => { +const CampaignSelection = ({ + onSelect, + formData, + formState, + props: { + sessionData, + employeeDetails, + allHierarchy, + isSubmitting + } +}) => {🧰 Tools
🪛 Biome (1.9.4)
[error] 25-25: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js(4 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundaries.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundariesDuplicate.js(2 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/CampaignConfig.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useBoundaryHome.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useBoundaryHome.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundaries.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundariesDuplicate.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/CampaignConfig.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js (1)
Pattern **/*.js: check
🪛 Biome (1.9.4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundariesDuplicate.js
[error] 183-204: 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] 213-213: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js
[error] 186-202: 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] 230-270: 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] 236-236: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 234-234: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 235-235: 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] 246-256: 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] 257-267: 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)
⏰ Context from checks skipped due to timeout of 10000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/CampaignConfig.js (1)
104-106: LGTM! Good improvement in prop passing pattern.The changes correctly move away from global state (window object) to proper React props, improving the component's predictability and testability.
Also applies to: 213-215
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (2)
68-68: LGTM! Well-structured configuration retrieval.The constant is properly defined with a fallback value, following the codebase's configuration management pattern.
246-247: LGTM! Proper dependency management.The campaign configuration correctly updates when hierarchy or employee data changes, with all dependencies properly declared in the dependency array.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js (1)
Line range hint
119-124: Review the necessity of executionCount effect.The current implementation has a potential issue:
- The effect runs on every render without dependencies
- The count limit of 5 seems arbitrary
- The effect updates the parent state which could cause unnecessary re-renders
Consider using a more deterministic approach or removing this if not necessary.
Would you like me to suggest an alternative implementation?
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundariesDuplicate.js (2)
6-6: LGTM! Good improvements to state management.The changes improve the component's state management and user experience by:
- Adding loading state for better UX
- Centralizing hierarchy data management
- Moving away from hook-based data fetching to more controlled data fetching
Also applies to: 14-27
166-167: LGTM! Good loading state handling.The loading state implementation improves user experience by showing a loading indicator during data fetching and preventing content flash.
Also applies to: 225-225
...ro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/CampaignConfig.js
Outdated
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/CampaignConfig.js
Outdated
Show resolved
Hide resolved
...web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js
Outdated
Show resolved
Hide resolved
.../web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js
Outdated
Show resolved
Hide resolved
...cro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useBoundaryHome.js
Outdated
Show resolved
Hide resolved
...o-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js
Outdated
Show resolved
Hide resolved
...o-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js
Show resolved
Hide resolved
...i-internals/packages/modules/campaign-manager/src/components/SelectingBoundariesDuplicate.js
Show resolved
Hide resolved
...i-internals/packages/modules/campaign-manager/src/components/SelectingBoundariesDuplicate.js
Outdated
Show resolved
Hide resolved
...i-internals/packages/modules/campaign-manager/src/components/SelectingBoundariesDuplicate.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 27
🔭 Outside diff range comments (6)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundaryComponent.js (4)
292-295: Missing Lodash Import for_.isEqualUsageThe code uses
_.isEqualin several places (e.g., within thehandleBoundaryChangefunction and in the state updater callback) to perform deep comparisons. However, lodash isn’t imported as_—onlymergeis imported from lodash on line 10. This will lead to runtime errors. You can fix this by either importing the entire lodash library as_or by importingisEqualexplicitly. For example:- import { merge } from "lodash"; + import _ from "lodash";Alternatively, if you prefer a more modular approach:
- // No additional import needed for isEqual + import isEqual from "lodash/isEqual";…and then update all calls from
_.isEqual(...)toisEqual(...).Also applies to: 321-332, 359-363
416-424: IncludeonSelectin useEffect DependenciesThe useEffect from lines 416–424 uses the
onSelectprop inside the setTimeout callback but does not includeonSelectin its dependency array. IfonSelectmight change, including it will prevent stale closures. Consider updating the dependency array to includeonSelect.
443-608: React List Rendering: MissingkeyPropsBoth the multi-select branch (lines ~443–573) and the dropdown branch (lines ~574–608) involve mapping over lists of boundary items without setting a unique
keyprop on the rendered elements (e.g., on the<LabelFieldPair>components). Adding a unique key (such as an ID or a combination of unique attributes) is recommended to help React efficiently update the list during re-renders.🧰 Tools
🪛 Biome (1.9.4)
[error] 466-466: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 506-506: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 584-584: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 464-464: 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] 504-504: 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] 582-582: 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)
483-485: Cleanup: Remove Commented-outonSelectHandlerIn the JSX for the multi-select dropdown (around lines 483–485), there’s a commented-out line for the
onSelecthandler. If this code is not needed, please remove it to reduce clutter and improve readability.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js (1)
23-53: Consider consolidating related state variables.The component has multiple related state variables that could be consolidated into a single state object for better state management.
- const [showToast, setShowToast] = useState(false); - const [loading, setLoading] = useState(true); - const [filteredProjectTypes, setFilteredProjectTypes] = useState([]); + const [state, setState] = useState({ + showToast: false, + loading: true, + filteredProjectTypes: [] + });🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
[error] 26-26: 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/campaign-manager/src/pages/employee/UpdateCampaign.js (1)
251-442: Refactor large useEffect for better maintainability.The large useEffect block handling API calls should be broken down into smaller, more focused functions.
+const handleCreatePayload = (draftData, totalFormData, hierarchyType) => { + let payloadData = { ...draftData }; + payloadData.hierarchyType = hierarchyType; + // ... rest of payload preparation + return payloadData; +}; + +const handleAPICall = async (payloadData, isCreate = false) => { + const mutateFunction = isCreate ? mutate : updateCampaign; + await mutateFunction(payloadData, { + onError: handleError, + onSuccess: handleSuccess, + onSettled: () => setIsDataCreating(false) + }); +}; + useEffect(async () => { if (shouldUpdate === true) { - if (filteredConfig?.[0]?.form?.[0]?.body?.[0]?.skipAPICall && !id) { - return; - } else if (filteredConfig?.[0]?.form?.[0]?.isLast) { - const reqCreate = async () => { - let payloadData = { ...draftData }; - // ... rest of the effect + if (!shouldMakeAPICall()) return; + + const payloadData = handleCreatePayload(draftData, totalFormData, hierarchyType); + if (compareIdentical(draftData, payloadData)) { + setCurrentKey(currentKey + 1); + return; + } + + await handleAPICall(payloadData, !id); } setShouldUpdate(false); }, [shouldUpdate]);🧰 Tools
🪛 Biome (1.9.4)
[error] 414-414: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
[error] 415-415: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
[error] 257-257: This let declares a variable that is only assigned once.
'payloadData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 322-322: This let declares a variable that is only assigned once.
'payloadData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 380-380: This let declares a variable that is only assigned once.
'payloadData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
♻️ Duplicate comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
122-136: 🛠️ Refactor suggestionAdd error handling for MDMS API response.
While the request configuration is well-structured, consider adding error handling for potential API failures.
Add error handling by wrapping the API call in try-catch blocks:
const { data: allHierarchy } = Digit.Hooks.useCustomAPIHook(reqAllHierarchy); +const { data: allHierarchy, error: hierarchyError } = Digit.Hooks.useCustomAPIHook(reqAllHierarchy); + +useEffect(() => { + if (hierarchyError) { + setShowToast({ + key: "error", + label: t("ES_ERROR_LOADING_HIERARCHY_DATA") + }); + } +}, [hierarchyError]);health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useBoundaryHome.js (1)
45-65: 🧹 Nitpick (assertive)Consider rethrowing or returning a structured error object instead of an empty array.
While returning
[]ensures the code doesn’t break at higher levels, it may mask underlying errors. Also, consider a proper logging utility rather than usingconsole.error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js(4 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundariesDuplicate.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundaryComponent.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/UpdateBoundaryWrapper.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/index.js(2 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useBoundaryHome.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useEmployeeHierarchyType.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js(2 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js(2 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateCampaign.js(1 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateCampaign.js
🧰 Additional context used
📓 Path-based instructions (11)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/index.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateCampaign.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useEmployeeHierarchyType.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundaryComponent.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/UpdateBoundaryWrapper.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useBoundaryHome.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundariesDuplicate.js (1)
Pattern **/*.js: check
📓 Learnings (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (1)
Learnt from: siddhant-nawale-egov
PR: egovernments/DIGIT-Frontend#741
File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/Module.js:15-15
Timestamp: 2024-11-10T20:01:49.000Z
Learning: When adding a new module code like "campaignmanager" to `moduleCode`, ensure that it is properly handled in all relevant conditional checks and function parameters across the codebase.
🪛 Biome (1.9.4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js
[error] 93-93: 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] 94-94: 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] 95-95: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateCampaign.js
[error] 70-70: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js
[error] 33-33: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
[error] 26-26: 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] 204-220: 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] 261-261: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 259-259: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 260-260: 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] 271-281: 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] 282-292: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/UpdateBoundaryWrapper.js
[error] 34-34: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js
[error] 85-85: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundariesDuplicate.js
[error] 25-25: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
[error] 167-178: 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] 187-187: 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)
⏰ Context from checks skipped due to timeout of 10000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (47)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js (3)
82-88: LGTM! The hook implementation is well-structured.The
useEmployeeHierarchyTypehook correctly handles hierarchy data fetching with proper loading state management.🧰 Tools
🪛 Biome (1.9.4)
[error] 85-85: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
91-93: LGTM! Efficient memoization of lowestHierarchy.The code correctly memoizes the lowestHierarchy value from rawData with proper dependency tracking.
68-80: Verify the HRMS context path configuration.The code correctly defines the
HRMS_CONTEXT_PATHconstant with a fallback value. The removal of the MDMS-based hierarchy schema fetching aligns with the new approach.Run this script to verify the constant's usage:
✅ Verification successful
HRMS context configuration verified.
The HRMS context constant is correctly defined with a fallback value and is consistently used across multiple files. The removal of the MDMS-based hierarchy schema fetching is aligned with the new approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for HRMS_CONTEXT_PATH usage in the codebase. rg -A 5 'HRMS_CONTEXT_PATH'Length of output: 12335
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useBoundaryHome.js (7)
22-22: Prefer a structured logging approach instead of console.error.This repeats a previous recommendation to replace console-based logging with a consistent and configurable logging utility.
40-40: Prefer a structured logging approach instead of console.error.This repeats a previous recommendation to replace console-based logging with a consistent and configurable logging utility.
71-72: Validate empty hierarchy responses.If
fetchHierarchiesreturns an empty array (due to an error or no data), ensure subsequent logic accounts for this case to avoid unexpected behavior.
95-95: Prefer a structured logging approach instead of console.error.This repeats a previous recommendation to replace console-based logging with a consistent and configurable logging utility.
101-101: Query key looks appropriate.Using
["boundaryData", hierarchyType, userName, tenantId]as the key is a clear approach for caching and refetching boundaries.
113-113: Good practice returning isFetching.Exposing
isFetchingalongsideisLoadinghelps distinguish between initial load and subsequent background refetches.
117-117: Export statement looks fine.No issues found. The hook is properly exported as the default.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/UpdateBoundaryWrapper.js (1)
89-120: Implementation looks good!The component implementation:
- Properly handles boundary selection state
- Provides clear user feedback about boundary removal restrictions
- Maintains good separation of concerns
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundariesDuplicate.js (8)
6-6: Import of new component looks good.No issues spotted. The import statement is well-structured and consistent with the existing style.
30-34: Good use ofuseMemo.The memoization to compute
lowestHierarchyandhierarchyTypehelps avoid redundant recomputations. No issues found here.
36-38: State initialization is clear.Declaring separate state variables for
boundaryHierarchyDataandloadingimproves code clarity. Good job.
66-66: Fetching mail configuration appears consistent.Leveraging
Digit.Hooks.useCustomMDMSis aligned with the existing pattern in this codebase. No concerns raised.
95-95: Invocation ofonSelectis appropriate.Passing the updated boundary parameters ensures global state synchronization. The implementation is straightforward.
150-160: Loader logic and conditional rendering look correct.The conditional check between
loadingand non-loading states is well-implemented, preventing partial renders.
167-178: Use a self-closing element when no children are rendered.This was previously flagged. If
<Wrapper>has no child elements, consider making it self-closing for cleaner JSX syntax.- ></Wrapper> + />🧰 Tools
🪛 Biome (1.9.4)
[error] 167-178: 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)
180-198: Add a key prop for array elements.This was previously noted. When passing elements as an array to the
additionalElementsprop, each item requires a uniquekeyto help React identify changes.- additionalElements={[ - <span style={{ color: "#505A5F" }}> + additionalElements={[ + <span key="boundary-info" style={{ color: "#505A5F" }}> {t("HCM_BOUNDARY_INFO")} ... </span>, ]}🧰 Tools
🪛 Biome (1.9.4)
[error] 187-187: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundaryComponent.js (2)
218-222: Dependency Array Enhancement in useEffectIn the useEffect starting on line 218, the callback depends on
boundaryOptions[parentRoot]but does not explicitly listparentRootas a dependency. To ensure the effect runs properly whenparentRootchanges, consider adding it explicitly to the dependency array.
1-1: 🧹 Nitpick (assertive)Questionable Import: Unused
useHook from ReactThe import statement on line 1 includes
use, which is not a standard React hook and isn’t used anywhere in this file. If this was added unintentionally or is meant for future implementation, please either remove it now or add a clarifying comment. A suggested change is:-import React, { useState, useEffect, Fragment, useRef, useMemo, use } from "react"; +import React, { useState, useEffect, Fragment, useRef, useMemo } from "react";✅ Verification successful
Final Response
Import Cleanup: Remove the unused React hook
useThe scan confirmed that the
usehook imported from React is not used within the file. Please remove it as suggested.
Final Response
Unused Import Correction: Remove
usefrom React importsThe refined search confirms that outside the import statement, there is no usage of the
usehook. Please remove it from the import line.
Unused Import Correction: Remove
usefrom React ImportsOur refined search confirms that the
usehook imported from React is not used anywhere in the file. It can be safely removed.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts or web queries found
Script:
#!/bin/bash # Exclude the import statement and search for any usage of the identifier "use" in the file. grep -v "^import" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundaryComponent.js | rg -w "use"Length of output: 149
Script:
#!/bin/bash # Use a refined regex to check for standalone usage of "use" (excluding the import statement) rg --no-heading -n -w "use" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundaryComponent.js | grep -v "import"Length of output: 162
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (3)
1-2: No concerns regarding these imports.
These added imports from the UI components library and React look standard and appropriate.
76-88: Handle loading and error state from the custom hook.
While the custom hook exposesisLoadinganderrorashierarchyLoadingand possibleerror, they are not used here. Consider conditionally handling loading states and errors to improve user experience and avoid edge-case failures.
91-93: Validate fallback for module code generation.
IfBOUNDARY_HIERARCHY_TYPEis empty or invalid, generating["boundary-"]might lead to unintended behavior. Confirm that your environment and data guarantee a valid non-empty boundary type before proceeding. Otherwise, implement appropriate handling.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/index.js (1)
22-22: Hook registration and import look good.
Importing theuseEmployeeHierarchyTypehook and adding it to thecampaignhooks object is straightforward and aligned with the existing structure.Also applies to: 52-52
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useEmployeeHierarchyType.js (5)
3-10: Initial states and constants setup appear fine.
Declaringdata,isLoading, anderrorstates, plusHRMS_CONTEXT_PATHandCONSOLE_MDMS_MODULENAME, is an appropriate approach for capturing hook-specific data and configuration.
11-45: Validate employee response and handle possible null scenarios.
While any error is caught in thetryblock, ifemployeeResponse?.Employeesis empty,employeebecomesundefined. Consider gracefully handling this scenario or returning early if no employees are found for the current user.
47-55: Check for multiple matching hierarchies.
Callingfind()will return the first matching hierarchy. Confirm whether this is desired or whether you need to accommodate multiple potential matches for an employee.
56-73: Good use of a result object and error boundary.
Packaging the hierarchy,employee, andallHierarchiesinto a single object is clean. Thetry/catchensures unhandled exceptions are minimized. Consider logging or storing a user-friendly error if the data is crucial to application flow.
75-94: Flexibleselectfunction and memo usage is a solid approach.
Allowing consumers to shape the returned data viaoptions.selectpromotes reusability. Caching withuseMemoalso helps optimize repeated renders.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js (6)
57-60: LGTM! Good use of useEffect for authorization and state updates.The effect hook correctly handles both authorization check and button state updates based on data availability.
89-97: LGTM! Good use of conditional rendering for InfoCard.The InfoCard component is properly rendered with appropriate conditions and styling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 93-93: 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] 94-94: 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] 95-95: 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)
49-50: LGTM! State management for button control.The state variable
disButtonis appropriately initialized and used to control button states based on boundary data availability.
57-60: LGTM! Authorization check implementation.The useEffect hook correctly checks for the BOUNDARY_MANAGER role and updates states based on data availability.
81-81: LGTM! Button disabled logic.The button's disabled state correctly considers multiple conditions: edit/create restrictions, boundary data availability, and user authorization.
98-109: LGTM! Toast notifications for user feedback.The Toast components provide clear feedback about boundary data and authorization status.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js (8)
54-87: 🛠️ Refactor suggestionMemoize getGroupsFromMatchingHierarchies for performance.
The function performs complex operations and should be memoized to prevent unnecessary recalculations.
- const getGroupsFromMatchingHierarchies = (employeeDetails, hierarchies) => { + const getGroupsFromMatchingHierarchies = useMemo(() => (employeeDetails, hierarchies) => { if (!employeeDetails?.assignments || !hierarchies?.length) { return []; } // ... rest of the function return Array.from(groups); - }; + }, []);Likely invalid or redundant comment.
54-87: 🧹 Nitpick (assertive)Optimize hierarchy filtering logic.
The
getGroupsFromMatchingHierarchiesfunction could be memoized to prevent unnecessary recalculations.+const getGroupsFromMatchingHierarchies = useMemo(() => { -const getGroupsFromMatchingHierarchies = (employeeDetails, hierarchies) => { if (!employeeDetails?.assignments || !hierarchies?.length) { return []; } // Get employee departments from assignments const employeeDepartments = new Set( employeeDetails.assignments .filter(assignment => assignment.department) .map(assignment => assignment.department.toUpperCase()) ); // Filter hierarchies where employee department matches hierarchy department const matchingHierarchies = hierarchies.filter(hierarchy => { const hierarchyDepartments = Array.isArray(hierarchy?.department) ? hierarchy?.department : [hierarchy?.department]; return hierarchyDepartments.some(dept => employeeDepartments.has(dept?.toUpperCase()) ); }); // Extract unique groups from matching hierarchies const groups = new Set( matchingHierarchies .filter(hierarchy => hierarchy.group) .flatMap(hierarchy => hierarchy.group) ); return Array.from(groups); -}; +}, [employeeDetails, hierarchies]);Likely invalid or redundant comment.
204-220:⚠️ Potential issueAdd keyboard accessibility to campaign type wrapper.
The click handler needs corresponding keyboard events for accessibility compliance.
<div className="campaign-type-wrapper" + role="button" + tabIndex={0} onClick={(e) => { if (props?.props?.sessionData?.HCM_CAMPAIGN_TYPE?.projectType && !canUpdate && source!=="microplan") { setShowPopUp(true); return; } return; }} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + if (props?.props?.sessionData?.HCM_CAMPAIGN_TYPE?.projectType && !canUpdate && source!=="microplan") { + setShowPopUp(true); + return; + } + } + }} onFocus={(e) => { if (props?.props?.sessionData?.HCM_CAMPAIGN_TYPE?.projectType && !canUpdate) { setShowPopUp(true); return; } return; }}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 204-220: 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)
259-293:⚠️ Potential issueFix React best practices in PopUp component.
Several React best practices issues need to be addressed in the PopUp component.
<PopUp className={"boundaries-pop-module"} type={"default"} heading={t("ES_CAMPAIGN_UPDATE_TYPE_MODAL_HEADER")} - children={[ - <div> - <CardText style={{ margin: 0 }}>{t("ES_CAMPAIGN_UPDATE_TYPE_MODAL_TEXT") + " "}</CardText> - </div>, - ]} > + <div> + <CardText style={{ margin: 0 }}>{`${t("ES_CAMPAIGN_UPDATE_TYPE_MODAL_TEXT")} `}</CardText> + </div> <div className="popup-button-wrapper"> <Button + key="back-button" className={"campaign-type-alert-button"} // ... other props /> <Button + key="submit-button" className={"campaign-type-alert-button"} // ... other props /> </div> </PopUp>Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 261-261: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 259-259: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 260-260: 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] 271-281: 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] 282-292: 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)
23-43: LGTM! State management and hooks setup.The component properly initializes states for loading, toast, and filtered data, and uses memoization for employee details and hierarchies.
🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
[error] 26-26: 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)
204-220: 🛠️ Refactor suggestionAdd keyboard accessibility to clickable div.
The div with click handler needs keyboard event handlers for accessibility.
<div className="campaign-type-wrapper" onClick={(e) => { if (props?.props?.sessionData?.HCM_CAMPAIGN_TYPE?.projectType && !canUpdate && source!=="microplan") { setShowPopUp(true); return; } return; }} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + if (props?.props?.sessionData?.HCM_CAMPAIGN_TYPE?.projectType && !canUpdate && source!=="microplan") { + setShowPopUp(true); + return; + } + } + }} + role="button" + tabIndex={0} onFocus={(e) => {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 204-220: 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)
259-293: 🛠️ Refactor suggestionFix React best practices in PopUp component.
Several React best practices need to be addressed:
- Missing key props in iterables
- Using string concatenation instead of template literals
- Passing children as props
<PopUp className={"boundaries-pop-module"} type={"default"} heading={t("ES_CAMPAIGN_UPDATE_TYPE_MODAL_HEADER")} - children={[ - <div> - <CardText style={{ margin: 0 }}>{t("ES_CAMPAIGN_UPDATE_TYPE_MODAL_TEXT") + " "}</CardText> - </div>, - ]} > + <div> + <CardText style={{ margin: 0 }}>{`${t("ES_CAMPAIGN_UPDATE_TYPE_MODAL_TEXT")} `}</CardText> + </div> <div className="popup-button-wrapper"> <Button + key="back-button" className={"campaign-type-alert-button"} // ... other props /> <Button + key="submit-button" className={"campaign-type-alert-button"} // ... other props /> </div> </PopUp>Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 261-261: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 259-259: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 260-260: 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] 271-281: 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] 282-292: 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)
54-87: 🧹 Nitpick (assertive)Consider optimizing hierarchy filtering.
The
getGroupsFromMatchingHierarchiesfunction performs multiple array operations. Consider memoizing the result to avoid unnecessary recalculations.+const memoizedGroups = useMemo( + () => getGroupsFromMatchingHierarchies(employeeDetails, hierarchies), + [employeeDetails, hierarchies] +);Likely invalid or redundant comment.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateCampaign.js (3)
67-73: LGTM! Good use of the new hook with proper data selection.The useEmployeeHierarchyType hook is well implemented with appropriate data selection.
🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
67-73: LGTM! Employee hierarchy hook implementation.The useEmployeeHierarchyType hook is properly implemented with data selection and loading state management.
🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
74-77: LGTM! Updated hierarchy logic.The lowestHierarchy calculation is correctly updated to use the new data structure from rawData.
...web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SetupCampaign.js
Show resolved
Hide resolved
...cro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useBoundaryHome.js
Show resolved
Hide resolved
...micro-ui-internals/packages/modules/campaign-manager/src/components/UpdateBoundaryWrapper.js
Show resolved
Hide resolved
...micro-ui-internals/packages/modules/campaign-manager/src/components/UpdateBoundaryWrapper.js
Show resolved
Hide resolved
...micro-ui-internals/packages/modules/campaign-manager/src/components/UpdateBoundaryWrapper.js
Show resolved
Hide resolved
...eb/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateCampaign.js
Show resolved
Hide resolved
...eb/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateCampaign.js
Show resolved
Hide resolved
.../web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js
Show resolved
Hide resolved
...o-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignType.js
Show resolved
Hide resolved
.../web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js
Outdated
Show resolved
Hide resolved
…gn-manager/src/Module.js
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.js`: check
**/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (2)
71-73: Remove or utilize the unused state variables.
They appear to have no references throughout the component, mirroring prior feedback.
118-124: Eliminate naming confusion betweenhierarchyDataandhierarchyRelationData.
The variablehierarchyRelationDatais passed ashierarchyDatawhile a separatehierarchyDatais also used to deriveemployeeDetails, which can be confusing.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js
Show resolved
Hide resolved
| // Fetch hierarchies using MDMS | ||
| const mdmsResponse = await Digit.CustomService.getResponse( | ||
| { | ||
| url: '/egov-mdms-service/v1/_search', |
There was a problem hiding this comment.
why mdms url is hardcoded?
@suryansh-egov
| const [filteredHierarchyType, setFilteredHierarchyType] = useState(null); | ||
| const [employeeData, setEmployeeData] = useState(null); | ||
| const [loading, setLoading] = useState(true); | ||
| const HRMS_CONTEXT_PATH = window?.globalConfigs?.getConfig("HRMS_CONTEXT_PATH") || "egov-hrms"; |
There was a problem hiding this comment.
why hrms context path is needed here since u have a custom hook do these activity
| @@ -33,6 +50,68 @@ const CampaignSelection = ({ onSelect, formData, formState, ...props }) => { | |||
| window.history.replaceState({}, "", url); | |||
| } | |||
|
|
|||
| const [filteredProjectTypes, setFilteredProjectTypes] = useState([]); | |||
| const getGroupsFromMatchingHierarchies = (employeeDetails, hierarchies) => { | |||
There was a problem hiding this comment.
dont put these logic in campaign type filtering use a function and do that logic inside it
| }],{select:(MdmsRes)=>MdmsRes},{ schemaCode: `${CONSOLE_MDMS_MODULENAME}.HierarchySchema` }); | ||
| const { data: mailConfig } = Digit.Hooks.useCustomMDMS(tenantId, CONSOLE_MDMS_MODULENAME, [{ name: "mailConfig" }],{select:(MdmsRes)=>MdmsRes},{ schemaCode: `${CONSOLE_MDMS_MODULENAME}.mailConfig` }); | ||
| // const hierarchyType = props?.props?.dataParams?.hierarchyType; | ||
| const employee = props?.props?.employeeDetails?.Employees?.[0]; |
| const fetchHierarchies = async (tenantId) => { | ||
| try { | ||
| const res = await Digit.CustomService.getResponse({ | ||
| url: `/egov-mdms-service/v1/_search`, |
Choose the appropriate template for your PR:
Feature PR
Feature Request
JIRA ID
Module
Description
Related Issues
Bugfix PR
Bugfix Request
JIRA ID
Module
Description
Root Cause
Related Issues
Release PR
Summary by CodeRabbit
Release Notes
New Features
Improvements
User Experience Enhancements