cleanup and roles from mdms v2 #1634
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple components in the campaign manager module. Key changes include restructuring the JSX layout 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 (
|
| @@ -453,8 +453,6 @@ const UpdateChecklist = () => { | |||
| ]} | |||
| sortFooterChildren={true} | |||
There was a problem hiding this comment.
can you make the same implementation in update checklist flow as well
|
|
||
| const [listsopt, setListsOpt] = useState([]); | ||
| const reqCriteria = { | ||
| url: `/mdms-v2/v2/_search`, |
There was a problem hiding this comment.
also get this url from global config
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range comments (7)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/checklistSearchConfig.js (1)
Line range hint
1-120: Consider refactoring configuration for better maintainability.Several improvements could make the configuration more maintainable:
- Extract translation prefixes to constants
- Remove commented code in the searchResult columns
- Consider moving hardcoded values like pagination limits to constants
Example refactor:
const TRANSLATION_PREFIXES = { ROLE: "ACCESSCONTROL_ROLES_ROLES_", CHECKLIST_TYPE: "HCM_CHECKLIST_TYPE_" }; const PAGINATION_DEFAULTS = { offset: 0, limit: 5 }; export const checklistSearchConfig = [ { // ... rest of the config apiDetails: { // ... other details Pagination: PAGINATION_DEFAULTS, }, sections: { searchResult: { uiConfig: { columns: [ { label: "CHECKLIST_ROLE", prefix: TRANSLATION_PREFIXES.ROLE, jsonPath: "data.role", translate: true }, { label: "CHECKLIST_TYPE", prefix: TRANSLATION_PREFIXES.CHECKLIST_TYPE, jsonPath: "data.checklistType", translate: true }, // ... other columns ] } } } } ];health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CreateQuestionContext.js (1)
Line range hint
1-183: Consider architectural improvements for better maintainability.
- Add error handling for localStorage operations:
const saveToStorage = (data) => { try { localStorage.setItem("questions", JSON.stringify(data)); } catch (error) { console.error("Failed to save questions:", error); // Consider implementing a fallback or user notification } };
Consider implementing an error boundary to gracefully handle runtime errors.
The component could benefit from being split into smaller, more focused components to reduce complexity and prop drilling.
Would you like assistance in implementing any of these architectural improvements?
🧰 Tools
🪛 Biome
[error] 42-45: 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)
[error] 46-51: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js (1)
Line range hint
12-24: Consider separating concerns for better maintainabilityThe component currently handles multiple responsibilities including:
- UI state management
- API interactions
- Error handling
- Localization
Consider:
- Moving API calls to a separate service layer
- Creating a custom hook for state management
- Implementing a dedicated error handling utility
This would improve maintainability and testability of the code.
Also applies to: 371-417
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateChecklist.js (3)
Line range hint
63-82: Add proper error handling in useEffect.The error handling in the data fetching useEffect is empty. This could lead to silent failures and poor user experience.
} catch (error) { + setSearching(false); + setShowToast({ + label: "ERROR_FETCHING_CHECKLIST_DATA", + isError: true + }); + console.error("Error fetching checklist data:", error); }🧰 Tools
🪛 Biome
[error] 475-475: 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)
Line range hint
42-48: Prevent memory leak in Toast timeout.The timeout for hiding the toast should be cleaned up when the component unmounts or when showToast changes.
useEffect(() => { if (showToast) { - setTimeout(closeToast, 5000); + const timer = setTimeout(closeToast, 5000); + return () => clearTimeout(timer); } }, [showToast]);🧰 Tools
🪛 Biome
[error] 475-475: 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)
Line range hint
10-31: Consider using reducer for complex state management.The component uses multiple useState hooks which could make state updates harder to track and maintain. Consider using useReducer for better state management.
Would you like me to help refactor the state management using useReducer?
🧰 Tools
🪛 Biome
[error] 475-475: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js (1)
Line range hint
19-37: Improve error handling in updateServiceDefinition.The error handling could be enhanced to provide better debugging capabilities and error reporting.
Consider this improvement:
const updateServiceDefinition = async (tenantId, newStatus, sdcode) => { try { const res = await Digit.CustomService.getResponse({ url: "/service-request/service/definition/v1/_update", body: { ServiceDefinition: { "tenantId": tenantId, "code": sdcode, "isActive": newStatus }, }, }); - if (res) { - } return res; } catch (error) { - // console.error("Error updating service definition:", error); + console.error("Error updating service definition:", error); + throw new Error(`Failed to update service definition: ${error.message}`); return null; } };
📜 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/components/CreateQuestion.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CreateQuestionContext.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/checklistSearchConfig.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js (5 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SearchChecklist.js (4 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateChecklist.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewChecklist.js (5 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CreateQuestion.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CreateQuestionContext.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/checklistSearchConfig.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SearchChecklist.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateChecklist.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewChecklist.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CreateQuestion.js
[error] 344-344: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
[error] 378-378: 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] 379-393: 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] 499-499: 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] 503-503: 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] 344-344: This let declares a variable that is only assigned once.
'dis' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 351-351: 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/CreateQuestionContext.js
[error] 42-45: 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)
[error] 46-51: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
[error] 76-84: 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)
[error] 95-95: 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)
[error] 96-96: 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)
[error] 97-97: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js
[error] 520-520: 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] 540-540: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SearchChecklist.js
[error] 55-55: This let declares a variable that is only assigned once.
'data' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateChecklist.js
[error] 475-475: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewChecklist.js
[error] 40-40: This let declares a variable that is only assigned once.
'temp_data' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 41-41: This let declares a variable that is only assigned once.
'formatted_data' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 42-42: This let declares a variable that is only assigned once.
'nvd' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 212-212: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
🔇 Additional comments (10)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/checklistSearchConfig.js (1)
16-16: LGTM! Schema code update looks good.The schema code update to "HCM-ADMIN-CONSOLE.Checklist_Templates" follows proper naming conventions and aligns with MDMS v2 migration.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js (3)
142-142: LGTM: Consistent isActive flag implementationThe addition of isActive flag in the question template maintains consistency with the payload structure.
153-153: Verify checklistCreateConfig handles "clear" parameterA new "clear" parameter has been added to checklistCreateConfig. Ensure this parameter is properly handled.
Let's check the implementation:
#!/bin/bash # Search for checklistCreateConfig implementation ast-grep --pattern 'function checklistCreateConfig($_, $_, $_) { $$$ }'
74-74: Verify schema code update impactThe schema code has been updated from a demo version to what appears to be a production version. This change could affect existing data retrieval.
Let's verify if there are any other files still using the old schema code:
✅ Verification successful
Let me try a broader search to better understand the schema usage patterns in the codebase.
This will help us understand:
- The usage pattern of HCM-ADMIN-CONSOLE schemas
- Any other checklist template schema variations that might exist
- The context around these schema usages
Based on the search results, I can now provide a final response:
Schema code update is consistent across the codebase
The schema code
HCM-ADMIN-CONSOLE.Checklist_Templatesis consistently used in both the CreateChecklist.js and checklistSearchConfig.js files. There are no remaining references to the old demo schema code (ChecklistTemplates_DEMO2), and the new schema code follows the same pattern as other HCM-ADMIN-CONSOLE schemas used throughout the codebase (as seen in the multiple examples of other schema usages likehierarchyConfig,adminSchema, etc.).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old schema code rg "ChecklistTemplates_DEMO2"Length of output: 75
Script:
#!/bin/bash # Search for similar schema patterns in the codebase rg "HCM-ADMIN-CONSOLE" -A 2 -B 2 # Also search for Checklist related schema patterns rg "Checklist_Templates|ChecklistTemplates" -A 2 -B 2Length of output: 41760
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateChecklist.js (1)
Line range hint
271-339: Add error boundary for data transformation.The data transformation logic in
transformQuestionsis complex and could fail silently. Consider adding error handling and validation.🧰 Tools
🪛 Biome
[error] 475-475: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SearchChecklist.js (2)
107-107: Verify mutation of 'checklistSearchConfig' objectAssigning
additionalDetailstochecklistSearchConfig[0]mutates the configuration object. IfchecklistSearchConfigis imported from a shared or constant configuration, mutating it might lead to unintended side effects in other parts of the application.Ensure that mutating
checklistSearchConfighere is intentional and does not affect other components that rely on this configuration.
213-213: LGTMThe
Steppercomponent is correctly configured with the appropriate props and should function as expected.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewChecklist.js (3)
21-21: LGTM!The introduction of
serviceCodevariable is correct and aligns with the codebase conventions.
129-131: Display Loader while data is loadingThe loading state is appropriately handled by displaying the
<Loader />component whenisLoadingis true.
214-220: Verify form submission handling before navigationIn the
onSubmithandler ofFormComposerV2, the form redirects to the update checklist page without handling or saving the form data. Please ensure that any user input is properly processed or validated before navigation to prevent potential loss of data.
| // schemaCode: "HCMadminconsole.checklisttemplates" | ||
| schemaCode: "HCM-ADMIN-CONSOLE.ChecklistTemplates_DEMO", | ||
| schemaCode: "HCM-ADMIN-CONSOLE.Checklist_Templates", | ||
| filters : {} | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove commented out schema code.
Clean up the old commented schema code to improve code readability.
- // schemaCode: "HCMadminconsole.checklisttemplates"
schemaCode: "HCM-ADMIN-CONSOLE.Checklist_Templates",📝 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.
| // schemaCode: "HCMadminconsole.checklisttemplates" | |
| schemaCode: "HCM-ADMIN-CONSOLE.ChecklistTemplates_DEMO", | |
| schemaCode: "HCM-ADMIN-CONSOLE.Checklist_Templates", | |
| filters : {} | |
| } | |
| schemaCode: "HCM-ADMIN-CONSOLE.Checklist_Templates", | |
| filters : {} | |
| } |
| case "CLEAR_DATA": | ||
| const newState = state.map(item => { | ||
| // Set isActive to false for all previous items | ||
| return { ...item, isActive: false }; | ||
| }); | ||
| const updatedState = [ | ||
| ...newState, | ||
| { | ||
| ...action.payload[0], | ||
| } | ||
| ]; | ||
| return updatedState; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add block scoping and payload validation to the CLEAR_DATA case.
The current implementation has potential issues:
- Switch case declarations need block scoping
- Missing payload validation
Apply this fix:
case "CLEAR_DATA": {
+ if (!action.payload?.[0]) {
+ return state;
+ }
const newState = state.map(item => ({
...item,
isActive: false
}));
return [
...newState,
{ ...action.payload[0] }
];
}Consider filtering out inactive items to improve memory efficiency:
case "CLEAR_DATA": {
if (!action.payload?.[0]) {
return state;
}
- const newState = state.map(item => ({
- ...item,
- isActive: false
- }));
+ const newState = state
+ .filter(item => item.isActive)
+ .map(item => ({
+ ...item,
+ isActive: false
+ }));
return [
...newState,
{ ...action.payload[0] }
];
}📝 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.
| case "CLEAR_DATA": | |
| const newState = state.map(item => { | |
| // Set isActive to false for all previous items | |
| return { ...item, isActive: false }; | |
| }); | |
| const updatedState = [ | |
| ...newState, | |
| { | |
| ...action.payload[0], | |
| } | |
| ]; | |
| return updatedState; | |
| case "CLEAR_DATA": { | |
| if (!action.payload?.[0]) { | |
| return state; | |
| } | |
| const newState = state | |
| .filter(item => item.isActive) | |
| .map(item => ({ | |
| ...item, | |
| isActive: false | |
| })); | |
| return [ | |
| ...newState, | |
| { ...action.payload[0] } | |
| ]; | |
| } |
🧰 Tools
🪛 Biome
[error] 42-45: 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)
[error] 46-51: 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)
| if(props?.props?.typeOfCall === "clear") | ||
| { | ||
| dispatchQuestionData({ | ||
| type: "CLEAR_DATA", | ||
| payload: props?.props?.data | ||
| }) | ||
|
|
||
| } | ||
| else{ | ||
| dispatchQuestionData({ | ||
| type: "UPDATE_QUESTION_DATA", | ||
| payload: props?.props?.data, | ||
| }); | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
Improve useEffect robustness and clarity.
The current implementation has several potential issues:
- The condition
props?.props?.data !== 0is unclear and could be improved - Deep prop access without proper null checks
- Missing cleanup function
Apply these improvements:
useEffect(() => {
- if (props?.props?.data !== 0) {
+ // Ensure we have valid data to process
+ if (!props?.props) {
+ return;
+ }
+
+ const { data, typeOfCall } = props.props;
+ if (!data) {
+ return;
+ }
+
setTypeOfCall(props?.props?.typeOfCall);
- if(props?.props?.typeOfCall === "clear") {
+ if(typeOfCall === "clear") {
dispatchQuestionData({
type: "CLEAR_DATA",
- payload: props?.props?.data
+ payload: data
});
} else {
dispatchQuestionData({
type: "UPDATE_QUESTION_DATA",
- payload: props?.props?.data,
+ payload: data,
});
}
- }
+
+ // Cleanup function
+ return () => {
+ setTypeOfCall(null);
+ };
}, [props?.props?.data]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(props?.props?.typeOfCall === "clear") | |
| { | |
| dispatchQuestionData({ | |
| type: "CLEAR_DATA", | |
| payload: props?.props?.data | |
| }) | |
| } | |
| else{ | |
| dispatchQuestionData({ | |
| type: "UPDATE_QUESTION_DATA", | |
| payload: props?.props?.data, | |
| }); | |
| } | |
| // Ensure we have valid data to process | |
| if (!props?.props) { | |
| return; | |
| } | |
| const { data, typeOfCall } = props.props; | |
| if (!data) { | |
| return; | |
| } | |
| setTypeOfCall(props?.props?.typeOfCall); | |
| if(typeOfCall === "clear") { | |
| dispatchQuestionData({ | |
| type: "CLEAR_DATA", | |
| payload: data | |
| }); | |
| } else { | |
| dispatchQuestionData({ | |
| type: "UPDATE_QUESTION_DATA", | |
| payload: data, | |
| }); | |
| } | |
| // Cleanup function | |
| return () => { | |
| setTypeOfCall(null); | |
| }; |
| {/* <TextInput | ||
| // style={{ maxWidth: "40rem" }} | ||
| name="title" | ||
| value={field?.title || ""} | ||
| onChange={(event) => handleUpdateField(event.target.value, "title", field.key, field.id)} | ||
| placeholder={"Type your questions here"} | ||
| className={"example"} | ||
| /> */} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clean up commented code and extract inline styles
Remove commented code and move inline styles to CSS for better maintainability.
- Remove commented TextInput component (lines 403-410)
- Extract inline styles:
-<div className="question-field" style={{ display: "flex", height: "3.5rem", gap: "1.5rem" }}>
+<div className="question-field question-field--layout">Add to CSS:
.question-field--layout {
display: flex;
height: 3.5rem;
gap: 1.5rem;
}Also applies to: 411-432
| {field?.isRegex && ( | ||
| <Dropdown | ||
| style={{ width: "70%" }} | ||
| t={t} | ||
| option={regexOption} | ||
| optionKey={"code"} | ||
| selected={field?.regex || ""} | ||
| select={(value) => { | ||
| handleUpdateField(value, "regex", field.key, field.id); | ||
| }} | ||
| props={{ fieldStyle: example }} | ||
| name="Short Answer" | ||
| value={field.value || ""} | ||
| onChange={(event) => handleUpdateField(event.target.value, "value", field.key, field.id)} | ||
| placeholder={""} | ||
| placeholder="Choose Regex" | ||
| /> | ||
| ) | ||
| } | ||
| {!dis && field.dependency && ( | ||
| <CreateQuestion | ||
| onSelect={onSelect} | ||
| className="subSection" | ||
| level={level + 1} | ||
| parent={field} | ||
| parentId={field.id} | ||
| initialQuestionData={initialQuestionData} // Pass sub-questions data to nested component | ||
| > | ||
| </CreateQuestion> | ||
| )} | ||
| )} | ||
| {(field?.type?.code === "SingleValueList" || field?.type?.code === "MultiValueList" || field?.type?.code === "Dropdown") && ( | ||
| <FieldSelector | ||
| t={t} | ||
| type={field?.type} | ||
| name={"value"} | ||
| value={field?.value || ""} | ||
| onChange={(event) => handleUpdateField(event.target.value, "value", field?.key, field.id)} | ||
| placeholder={"Answer"} | ||
| dispatchQuestionData={dispatchQuestionData} | ||
| field={field} | ||
| subQclassName="subSection" | ||
| subQlevel={level + 1} | ||
| subQparent={field} | ||
| subQparentId={field.id} | ||
| subQinitialQuestionData={initialQuestionData} | ||
| typeOfCall={typeOfCall} | ||
| /> | ||
| )} | ||
| { | ||
| (field?.type?.code === "Short Answer") && ( | ||
| <FieldV1 | ||
| disabled="true" | ||
| className="example" | ||
| type={"textarea"} | ||
| populators={{ | ||
| resizeSmart: true | ||
| }} | ||
| props={{ fieldStyle: example }} | ||
| name="Short Answer" | ||
| value={field.value || ""} | ||
| onChange={(event) => handleUpdateField(event.target.value, "value", field.key, field.id)} | ||
| placeholder={""} | ||
| /> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract regex and field selector logic into separate components
Consider extracting the regex dropdown and field selector logic into separate components to improve maintainability and reusability.
Create new components:
RegexSelector.jsfor the regex dropdownAnswerField.jsfor the short answer field
This will make the code more modular and easier to test.
...b/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SearchChecklist.js
Outdated
Show resolved
Hide resolved
| useEffect(() => { | ||
| setCampaignName(campaignName); | ||
| }, campaignName); |
There was a problem hiding this comment.
Remove unnecessary useEffect hook or correct its dependency array
The useEffect hook is setting campaignName to its current value and may be unnecessary. Additionally, the dependency array should be an array, but currently, campaignName is passed directly.
Consider removing the useEffect hook if it's not needed, or adjust the dependency array to be an array.
Proposed fix:
-useEffect(() => {
- setCampaignName(campaignName);
-}, campaignName);
+// Remove this useEffect if it's not needed📝 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.
| useEffect(() => { | |
| setCampaignName(campaignName); | |
| }, campaignName); |
| // <div> | ||
| // <CardText style={{ margin: 0 }}>{"testing" + " "}</CardText> | ||
| // </div>, | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unnecessary empty 'children' prop
The children prop in the PopUp component is passed an empty array, which is unnecessary. Omitting the children prop simplifies the code without affecting functionality.
Apply this diff to fix the issue:
<PopUp
className={"custom-pop-up"}
type={"default"}
heading={t("CHECKLIST_PREVIEW")}
- children={[
-
- ]}
onOverlayClick={() => {
setShowPopUp(false);
}}
onClose={() => {
setShowPopUp(false);
}}
footerChildren={[ /* ... */ ]}
sortFooterChildren={true}
>Committable suggestion was skipped due to low confidence.
...web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewChecklist.js
Outdated
Show resolved
Hide resolved
...web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewChecklist.js
Outdated
Show resolved
Hide resolved
…gn-manager/src/pages/employee/ViewChecklist.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…gn-manager/src/pages/employee/SearchChecklist.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…gn-manager/src/pages/employee/ViewChecklist.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/checklistSearchConfig.js (1)
Line range hint
46-47: Clean up commented code blocks throughout the file.There are several instances of commented-out code that should be removed to improve code maintainability and readability:
- Commented options in dropdown configs
- Commented columns configuration
- Commented tenantId
Would you like me to provide a diff showing all the cleanup changes?
Also applies to: 54-55, 66-71, 76-77
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateChecklist.js (3)
Line range hint
27-29: 'module' variable is used before it's assignedThe
modulevariable is used in the call touseUpsertLocalisationbefore it is assigned a value. This can lead tomodulebeingundefinedduring the function call.Apply this diff to assign
modulebefore its usage:+ const module = "hcm-checklist"; const { mutateAsync: localisationMutateAsync } = Digit.Hooks.campaign.useUpsertLocalisation(tenantId, module, locale); - module = "hcm-checklist";🧰 Tools
🪛 Biome
[error] 27-27: This let declares a variable that is only assigned once.
'locale' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
Line range hint
200-216: DuplicatecreateQuestionObjectfunction declarationsThere are two functions named
createQuestionObjectdefined in the code. This can cause conflicts or unexpected behaviors. Consider removing or renaming one of them to avoid confusion and potential errors.🧰 Tools
🪛 Biome
[error] 27-27: This let declares a variable that is only assigned once.
'locale' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
Line range hint
339-341: Replace placeholder condition with actual success checkIn the
onSubmitfunction, the conditionif (data.success)includes a comment indicating it needs replacement. Ensure you use the correct condition to check if the API call was successful, based on the actual structure of the response frommutateAsync.🧰 Tools
🪛 Biome
[error] 27-27: This let declares a variable that is only assigned once.
'locale' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/checklistSearchConfig.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/services/createTypeOfChecklist.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js (5 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SearchChecklist.js (4 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateChecklist.js (4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/checklistSearchConfig.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/services/createTypeOfChecklist.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SearchChecklist.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateChecklist.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js
[error] 523-523: 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] 543-543: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SearchChecklist.js
[error] 56-56: This let declares a variable that is only assigned once.
'data' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 78-78: This let declares a variable that is only assigned once.
'data' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/UpdateChecklist.js
[error] 72-72: This let declares a variable that is only assigned once.
'temp_data' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 73-73: This let declares a variable that is only assigned once.
'formatted_data' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 74-74: This let declares a variable that is only assigned once.
'nvd' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 459-459: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
🔇 Additional comments (7)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/checklistSearchConfig.js (4)
3-3: LGTM! Well-implemented configuration handling.The implementation correctly retrieves the MDMS V2 context path from global configs with a proper fallback value, using safe optional chaining.
9-9: LGTM! Good use of configuration-driven endpoint.The service name is now properly derived from the configured MDMS context path, making it more maintainable.
17-17: LGTM! Schema code update is correct.The updated schema code aligns with the expected format for HCM admin console checklist templates.
3-3: Verify MDMS v2 migration impact.The changes indicate a migration from MDMS v1 to v2. Let's verify that all related components are consistently updated.
Also applies to: 9-9, 17-17
✅ Verification successful
Let me analyze the results and generate a final response based on the verification performed.
MDMS v2 migration appears consistent in the codebase
The verification results show:
- No instances of old MDMS v1 endpoints (
/mdms/v1/_search) were found- The
MDMS_V2_CONTEXT_PATHconfiguration is consistently used across relevant files:
- In campaign-manager module's configuration and pages
- In the core libraries (urls.js)
- The fallback value "mdms-v2" is consistently set across all usages
The changes are properly aligned with the MDMS v2 migration, and no inconsistencies were detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining MDMS v1 endpoints or inconsistent schema codes # Test 1: Search for old MDMS v1 endpoints echo "Checking for MDMS v1 endpoints..." rg -l '/mdms/v1/_search' # Test 2: Check for inconsistent schema codes echo "Checking for inconsistent schema codes..." rg -l 'HCMadminconsole\.checklisttemplates' # Test 3: Verify consistent usage of MDMS_V2_CONTEXT_PATH config echo "Checking MDMS_V2_CONTEXT_PATH usage..." rg 'MDMS_V2_CONTEXT_PATH'Length of output: 1478
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/CreateChecklist.js (3)
145-145: LGTM: Form data handling improvementsThe addition of
isActiveflag and clear parameter enhances the form reset functionality.Also applies to: 156-156
523-523: Self-closing div elements issueThis issue has been previously identified in past reviews.
Also applies to: 543-543
🧰 Tools
🪛 Biome
[error] 523-523: 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)
63-77: Verify schema code update across the codebaseThe schema code has been updated from demo to production. Let's verify this change is consistent across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
Schema code update is consistent across the codebase
The schema code
HCM-ADMIN-CONSOLE.Checklist_Templatesis consistently used in both relevant files:
CreateChecklist.js: For creating new checklistschecklistSearchConfig.js: For search configurationNo instances of the old schema code
ChecklistTemplates_DEMO2were found, confirming a complete migration to the new schema.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old schema code rg "ChecklistTemplates_DEMO2" # Search for all occurrences of the new schema code to ensure consistency rg "Checklist_Templates"Length of output: 405
| @@ -1,7 +1,9 @@ | |||
| const createTypeOfChecklist = async (req, tenantId) => { | |||
There was a problem hiding this comment.
Remove unused tenantId parameter.
The function accepts a tenantId parameter but never uses it. Either remove the unused parameter or document why it's needed for future use.
-const createTypeOfChecklist = async (req, tenantId) => {
+const createTypeOfChecklist = async (req) => {Committable suggestion was skipped due to low confidence.
| @@ -1,7 +1,9 @@ | |||
| const createTypeOfChecklist = async (req, tenantId) => { | |||
| const mdms_context_path = window?.globalConfigs?.getConfig("MDMS_V2_CONTEXT_PATH") || "mdms-v2"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving configuration retrieval outside the function.
The MDMS context path configuration is retrieved on every function call. Consider moving this to a module-level constant or a configuration hook for better performance and reusability.
+const MDMS_CONTEXT_PATH = window?.globalConfigs?.getConfig("MDMS_V2_CONTEXT_PATH") || "mdms-v2";
+
const createTypeOfChecklist = async (req, tenantId) => {
- const mdms_context_path = window?.globalConfigs?.getConfig("MDMS_V2_CONTEXT_PATH") || "mdms-v2";
-
try {
const response = await Digit.CustomService.getResponse({
- url: `/${mdms_context_path}/v1/_search`,
+ url: `/${MDMS_CONTEXT_PATH}/v1/_search`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mdms_context_path = window?.globalConfigs?.getConfig("MDMS_V2_CONTEXT_PATH") || "mdms-v2"; | |
| const MDMS_CONTEXT_PATH = window?.globalConfigs?.getConfig("MDMS_V2_CONTEXT_PATH") || "mdms-v2"; | |
| const mdms_context_path = window?.globalConfigs?.getConfig("MDMS_V2_CONTEXT_PATH") || "mdms-v2"; |
| try { | ||
| const response = await Digit.CustomService.getResponse({ | ||
| url: "/mdms-v2/v1/_search", | ||
| url: `/${mdms_context_path}/v1/_search`, |
There was a problem hiding this comment.
Validate MDMS context path to prevent path injection.
The URL is constructed using a configurable value without validation. This could potentially lead to path injection if the configuration is compromised.
Add validation for the context path:
+const validateContextPath = (path) => {
+ return /^[a-zA-Z0-9-]+$/.test(path) ? path : "mdms-v2";
+};
+
const createTypeOfChecklist = async (req, tenantId) => {
- const mdms_context_path = window?.globalConfigs?.getConfig("MDMS_V2_CONTEXT_PATH") || "mdms-v2";
+ const mdms_context_path = validateContextPath(window?.globalConfigs?.getConfig("MDMS_V2_CONTEXT_PATH"));Committable suggestion was skipped due to low confidence.
| // useEffect(() => { | ||
| // setListsOpt(HCM?.HCM?.CHECKLIST_TYPES?.map((item) => ({ list: `HCM_CHECKLIST_TYPE_${item.code}` }))); | ||
| // }, [HCM]); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove commented-out code if it's no longer needed
The code in lines 91-93 is commented out and may not be necessary. Removing unused code enhances maintainability and reduces clutter.
| const { isLoading, data: HCM, isFetching } = Digit.Hooks.useCustomAPIHook(reqCriteriaResource); | ||
| const { isLoading, data: HCM, isFetching } = Digit.Hooks.useCustomAPIHook(reqCriteria1); | ||
| useEffect(() => { | ||
| let data = HCM?.mdms; |
There was a problem hiding this comment.
Use 'const' instead of 'let' for 'data' variable
The variable data is assigned once and never reassigned. Using const improves code readability and indicates that the variable will not change.
Apply this diff to fix the issue:
-let data = HCM?.mdms;
+const data = HCM?.mdms;📝 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.
| let data = HCM?.mdms; | |
| const data = HCM?.mdms; |
🧰 Tools
🪛 Biome
[error] 78-78: This let declares a variable that is only assigned once.
'data' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
| return ( | ||
| <div> | ||
| {submitting && <Loader />} | ||
| {/* {submitting && <Loader />} */} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unnecessary commented-out code
It's advisable to remove commented-out code to keep the codebase clean and maintainable.
| select: (res) => { | ||
| if (res?.ServiceDefinitions?.[0]?.attributes) { | ||
| setSearching(false); | ||
| let temp_data = res?.ServiceDefinitions?.[0]?.attributes | ||
| let formatted_data = temp_data.map((item) => item.additionalDetails); | ||
| let nvd = formatted_data.filter((value, index, self) => | ||
| index === self.findIndex((t) => t.id === value.id) | ||
| ); | ||
| setViewData(nvd); | ||
| // setViewData(formatted_data); | ||
|
|
||
| ); | ||
| return nvd; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use 'const' instead of 'let' for variables that are not reassigned
Variables temp_data, formatted_data, and nvd are declared with let but are never reassigned. Using const for variables that are not reassigned helps prevent accidental reassignments and improves code readability.
Apply this diff to change let to const:
select: (res) => {
if (res?.ServiceDefinitions?.[0]?.attributes) {
- let temp_data = res?.ServiceDefinitions?.[0]?.attributes
- let formatted_data = temp_data.map((item) => item.additionalDetails);
- let nvd = formatted_data.filter((value, index, self) =>
+ const temp_data = res?.ServiceDefinitions?.[0]?.attributes
+ const formatted_data = temp_data.map((item) => item.additionalDetails);
+ const nvd = formatted_data.filter((value, index, self) =>
index === self.findIndex((t) => t.id === value.id)
);
return nvd;📝 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.
| select: (res) => { | |
| if (res?.ServiceDefinitions?.[0]?.attributes) { | |
| setSearching(false); | |
| let temp_data = res?.ServiceDefinitions?.[0]?.attributes | |
| let formatted_data = temp_data.map((item) => item.additionalDetails); | |
| let nvd = formatted_data.filter((value, index, self) => | |
| index === self.findIndex((t) => t.id === value.id) | |
| ); | |
| setViewData(nvd); | |
| // setViewData(formatted_data); | |
| ); | |
| return nvd; | |
| select: (res) => { | |
| if (res?.ServiceDefinitions?.[0]?.attributes) { | |
| const temp_data = res?.ServiceDefinitions?.[0]?.attributes | |
| const formatted_data = temp_data.map((item) => item.additionalDetails); | |
| const nvd = formatted_data.filter((value, index, self) => | |
| index === self.findIndex((t) => t.id === value.id) | |
| ); | |
| return nvd; |
🧰 Tools
🪛 Biome
[error] 72-72: This let declares a variable that is only assigned once.
'temp_data' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 73-73: This let declares a variable that is only assigned once.
'formatted_data' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 74-74: This let declares a variable that is only assigned once.
'nvd' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
Summary by CodeRabbit
Release Notes
New Features
CreateQuestionContextcomponent for better handling of question data.MobileChecklistcomponent for improved checklist preview in theUpdateChecklistcomponent.Improvements
SearchChecklistandViewChecklist.CreateQuestioncomponent.UICustomizationsconfiguration.UpdateChecklistcomponent for clearer distinction between submission types.Bug Fixes
UpdateChecklistcomponent.Chores