Formula screens rework, cascading changes on formula deletion #1682
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant modifications across several components, primarily focusing on the Changes
Possibly related PRs
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: 19
🧹 Outside diff range comments (7)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js (5)
Line range hint
191-196: Potential memory leak in useEffect cleanup.The effect hook adds event listeners but the cleanup function doesn't remove them with the same parameters.
useEffect(() => { window.addEventListener("verticalStepper", moveToPreviousStep); return () => { - window.removeEventListener("verticalStepper", moveToPreviousStep); + window.removeEventListener("verticalStepper", moveToPreviousStep, { capture: false }); }; }, [internalKey]);
Line range hint
216-223: Remove unbounded execution count effect.The useEffect without dependencies will run on every render and has a manual counter to limit executions. This is an anti-pattern.
-useEffect(() => { - if (executionCount < 5) { - onSelect(customProps.name, {assumptionValues}) - setExecutionCount((prevCount) => prevCount + 1); - } - }); +useEffect(() => { + onSelect(customProps.name, {assumptionValues}); +}, [customProps.name, assumptionValues]);
Line range hint
238-255: Optimize assumption initialization logic.The current implementation may cause unnecessary re-renders due to state updates in useEffect.
useEffect(() => { - const initialAssumptions = filteredAssumptions.map(item => ({ - source: "MDMS", - category: null, - key: item, - value: null - })); - - // Create a set of existing keys for quick lookup - const existingKeys = new Set(assumptionValues.map(assumption => assumption.key)); - - // Filter out initialAssumptions to avoid duplicates and deleted assumptions - const newAssumptions = initialAssumptions.filter(assumption => - !existingKeys.has(assumption.key) && - !deletedAssumptions.includes(assumption.key) - ); - - // Update state only with non-duplicate assumptions - setAssumptionValues(prev => [...prev, ...newAssumptions]); + const existingKeys = new Set(assumptionValues.map(assumption => assumption.key)); + const deletedKeys = new Set(deletedAssumptions); + + const newAssumptions = filteredAssumptions.reduce((acc, item) => { + if (!existingKeys.has(item) && !deletedKeys.has(item)) { + acc.push({ + source: "MDMS", + category: null, + key: item, + value: null + }); + } + return acc; + }, []); + + if (newAssumptions.length > 0) { + setAssumptionValues(prev => [...prev, ...newAssumptions]); + } }, [filteredAssumptions]);
Line range hint
84-117: Improve error handling in API call.The error handling in the updateResources API call is incomplete. Errors are only logged to console without proper user feedback.
updateResources({ config:{ name:"SUB_HYPOTHESIS" }, assumptionsToUpdate },{ onSuccess: (data) => { if (internalKey < assumptionCategories.length) { setInternalKey((prevKey) => prevKey + 1); } refetchPlan(); + setShowToast({ + key: "success", + label: t("ASSUMPTIONS_UPDATED_SUCCESSFULLY"), + transitionTime: 3000 + }); }, onError: (error, variables) => { console.error(error) - // setShowToast(({ key: "error", label: error?.message ? error.message : t("FAILED_TO_UPDATE_RESOURCE") })) + setShowToast({ + key: "error", + label: error?.message || t("FAILED_TO_UPDATE_RESOURCE"), + transitionTime: 3000 + }); }, })
Line range hint
269-307: Improve accessibility in component structure.The component's UI structure needs accessibility improvements:
- Missing ARIA labels
- No keyboard navigation support for stepper
<div style={{ display: "flex",gap:"2rem" }}> - <div className="card-container"> + <div className="card-container" role="complementary" aria-label={t("ASSUMPTIONS_NAVIGATION")}> <Card className="card-header-timeline"> - <TextBlock subHeader={t("ESTIMATION_ASSUMPTIONS")} subHeaderClassName={"stepper-subheader"} wrapperClassName={"stepper-wrapper"} /> + <TextBlock + subHeader={t("ESTIMATION_ASSUMPTIONS")} + subHeaderClassName={"stepper-subheader"} + wrapperClassName={"stepper-wrapper"} + role="heading" + aria-level="2" + /> </Card> <Card className="stepper-card"> <Stepper customSteps={assumptionCategories.map(category => category.category)} currentStep={internalKey} - onStepClick={() => null} + onStepClick={handleStepClick} direction={"vertical"} + aria-label={t("ASSUMPTIONS_STEPPER")} /> </Card> </div>health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UICustomizations.js (2)
Line range hint
503-524: Enhance popup handling for better UX and memory management.While the popup implementation is good, consider these improvements:
- Add cleanup for any async operations when the popup closes
- Add loading state during async operations
- Consider using a portal for better modal rendering
Here's a suggested implementation:
return ( <> <ButtonNew className="" icon="ArrowForward" iconFill="" isSuffix label={t("MICROPLAN_ASSIGN")} onClick={() => setShowPopup(true)} options={[]} optionsKey="" size="medium" style={{}} title="" variation="secondary" /> {showPopup && ( + <Portal> <FacilityPopUp details={row} + isLoading={false} // Add loading state onClose={() => { + // Cleanup any subscriptions/async operations here setShowPopup(false); }} /> + </Portal> )} </> );
Line range hint
1-524: Refactor common patterns into reusable utilities.There's duplicate code for handling NA values and text wrapping styles across different configurations. Consider extracting these into reusable utilities.
Create utility functions:
// utils.js export const renderWithWordWrap = (value, t) => { if (value && value !== "NA") { return ( <div style={{ maxWidth: "15rem", wordWrap: "break-word", whiteSpace: "normal", overflowWrap: "break-word", }} > <p>{t(value)}</p> </div> ); } return ( <div> <p>{t("ES_COMMON_NA")}</p> </div> ); };Then use it in configurations:
- if (value && value !== "NA") { - return ( - <div - style={{ - maxWidth: "15rem", - wordWrap: "break-word", - whiteSpace: "normal", - overflowWrap: "break-word", - }} - > - <p>{t(value)}</p> - </div> - ); - } else { - return ( - <div> - <p>{t("ES_COMMON_NA")}</p> - </div> - ); - } + return renderWithWordWrap(value, t);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (6 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js (9 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UICustomizations.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js (9 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UICustomizations.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js
[error] 230-235: 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] 237-237: 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] 257-262: 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] 264-275: 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] 277-277: 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] 400-409: 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] 432-437: 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] 440-446: 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] 447-457: 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] 454-454: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
[error] 459-462: 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] 464-464: 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] 584-584: 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)
🔇 Additional comments (4)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js (1)
20-20: 🧹 Nitpick (assertive)Verify data persistence across page reloads.
The change from
customProps?.sessionDatatoDigit.SessionStorage.get("MICROPLAN_DATA")could lead to data loss if session storage is cleared or expires. Consider implementing a fallback mechanism.Consider implementing a more robust state management solution:
- Use Redux/Context for global state management
- Implement data persistence strategy (localStorage backup)
- Add error boundaries for handling storage-related failures
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (1)
18-20: 🧹 Nitpick (assertive)Consider adding a fallback mechanism for session storage
While using SessionStorage centralizes data management, consider adding a fallback mechanism in case the session storage is cleared or corrupted.
const [formulaConfigValues, setFormulaConfigValues] = useState( - Digit.SessionStorage.get("MICROPLAN_DATA")?.FORMULA_CONFIGURATION?.formulaConfiguration?.formulaConfigValues || [] + Digit.SessionStorage.get("MICROPLAN_DATA")?.FORMULA_CONFIGURATION?.formulaConfiguration?.formulaConfigValues || customProps?.formulaConfigValues || [] );health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js (2)
423-430:⚠️ Potential issueWrap 'case' clause in a block to prevent variable leakage
Variables such as
fetchedPlanForFormuladeclared within thiscaseclause can be accessed by other clauses. Enclosing thiscasein braces{}ensures variables are scoped correctly.Apply this diff:
case "FORMULA_CONFIGURATION": { if ( !totalFormData?.FORMULA_CONFIGURATION?.formulaConfiguration?.formulaConfigValues.every( (row) => row.category && row.output && row.input && row.operatorName && row.assumptionValue ) ) { setShowToast({ key: "error", label: "ERR_FORMULA_MANDATORY" }); return; } // rest of the code... }Likely invalid or redundant comment.
165-186: 🛠️ Refactor suggestionSimplify conditional assignment of
planObjectandcampaignObjectInstead of initializing
planObjectandcampaignObjectas empty objects and then reassigning them, consider declaring them within the conditional blocks where they are assigned. This improves readability and prevents unintended side effects.Apply this diff:
- let planObject = {}; - let campaignObject = {}; if (microplanId) { + const planObject = await searchPlanConfig({ PlanConfigurationSearchCriteria: { tenantId, id: microplanId, }, }); } if (campaignId) { + const campaignObject = await searchCampaignConfig({ CampaignDetails: { tenantId: tenantId, ids: [campaignId], }, }); }Likely invalid or redundant comment.
...-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
Show resolved
Hide resolved
...-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
Show resolved
Hide resolved
...-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
Show resolved
Hide resolved
...-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
Show resolved
Hide resolved
...-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
Show resolved
Hide resolved
.../micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js
Show resolved
Hide resolved
.../micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js
Show resolved
Hide resolved
.../micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js
Show resolved
Hide resolved
.../micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js
Show resolved
Hide resolved
.../micro-ui-internals/packages/modules/microplan/src/hooks/services/createUpdatePlanProject.js
Show resolved
Hide resolved
nipunarora-eGov
dismissed
coderabbitai[bot]’s stale review
resolved critical ones
Choose the appropriate template for your PR:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores