Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hcmpre 1290 #1834
Hcmpre 1290 #1834
Changes from 4 commits
3f88737
34c2409
c075921
a86b7f7
c79c4fd
f076ed0
978884b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Consider extracting URL parameter logic to a custom hook
The URL parameter handling could be moved to a custom hook (e.g.,
useCampaignType
) to:Example implementation:
Then in your component:
Also applies to: 147-147
🧹 Nitpick (assertive)
Add validation for the productType URL parameter
The
productType
parameter from URL should be validated to ensure it only accepts expected values (e.g., "microplan" or other valid types).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Remove unnecessary whitespace
Remove the empty line with whitespace after the disabled prop.
disabled = {productType === "microplan"} -
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify correct assignment of
status
inPlanConfigurationSearchCriteria
At line 317,
data.body.PlanConfigurationSearchCriteria.status = status?.status;
. Sincestatus
is destructured fromdata?.state?.searchForm || {}
, ensure thatstatus?.status
is the intended value. It might need to bestatus?.code
or simplystatus
.Please confirm that the correct value is being assigned to
status
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid duplicate assignments to
PlanConfigurationSearchCriteria.name
In the
preProcess
method,data.body.PlanConfigurationSearchCriteria.name
is assigned twice, at lines 313 and 318. This could lead to unintended behavior or confusion.Consider consolidating the assignments to ensure only one definitive value is set:
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Use template literals instead of string concatenation
Line 348 concatenates strings using
+
. For better readability and to adhere to best practices, use template literals.Modify line 348 as follows:
📝 Committable suggestion
🧰 Tools
🪛 Biome
[error] 348-348: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Clarify CM_DRAFT_TYPE implementation and add documentation
The new CM_DRAFT_TYPE column uses parentId without clear documentation. Consider:
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Remove unused variable declaration
The
mdms_context_path
variable is declared but never used in the configuration.- const mdms_context_path = window?.globalConfigs?.getConfig("MDMS_V2_CONTEXT_PATH") || "mdms-v2";
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace hard-coded values with dynamic values
The configuration contains hard-coded values for
tenantId
anduserUuid
. These should be replaced with dynamic values from the current context.📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just config, moduleName is differnt for microplan UI and here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Use proper i18n label keys
The form labels should follow proper i18n naming conventions for better internationalization support.
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Remove commented code
Remove the commented out
resultsJsonPath
line as it's no longer needed.- // resultsJsonPath: "mdms",
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
🛠️ Refactor suggestion
Remove commented hook definition and update hook name
The custom hook
useSavedMicroplans
exists and is properly implemented in the codebase. However, I noticed that the root level hook name should beuseSavedMicroplansWithCampaign
to match other config files.microplanv1.useSavedMicroplansWithCampaign
to maintain consistency withMyMicroplanSearchConfig.js
🔗 Analysis chain
Remove duplicate custom hook definition
The custom hook is defined twice - once commented out inside
sections
and once at the root level. Remove the commented version to avoid confusion.- // customHookName: "microplanv1.useSavedMicroplans",
Also, verify that the custom hook exists and is properly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 18341
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Fix undefined tabData state
The
onTabChange
function referencestabData
andsetTabData
but these are not defined in the component. The state needs to be initialized using useState hook.onTabChange
function as it's currently not being used by any tab component in the rendered JSXconst [tabData, setTabData] = useState([]);
🔗 Analysis chain
Fix incomplete tab implementation
The onTabChange function has several issues:
Let me search for the state definitions and React imports in the specific file to verify if tabData is properly initialized.
Let me check the complete file content to see how tabData is initialized and used in the component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 88832
Script:
Length of output: 806
Script:
Length of output: 1947
🧰 Tools
🪛 Biome
[error] 24-24: 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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Use self-closing tag for InboxSearchComposer.
Since the InboxSearchComposer component doesn't have any children, it should use a self-closing tag.
📝 Committable suggestion
🧰 Tools
🪛 Biome
[error] 20-27: 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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Consider using a custom hook for URL params.
The URL parameter handling is scattered across the component. Consider extracting URL parameter handling into a custom hook for better reusability and maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add breadcrumb entry for the new microplan route.
The new route for MyMicroplans is missing a corresponding breadcrumb entry in the CampaignBreadCrumb component's crumbs array. This will affect navigation consistency.
Add the following entry to the crumbs array in CampaignBreadCrumb: