toast being changed #1733
toast being changed #1733
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve updates to the user interface and logic in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range comments (3)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js (1)
Line range hint
233-233: Inconsistent error message label.The error message label at line 233 still uses
t("ERR_MANDATORY_FIELD")while the same validation at line 107 usest("HYP_ERR_MANDATORY_FIELD"). This inconsistency should be fixed to maintain a uniform user experience.Apply this diff to fix the inconsistency:
- label: t("ERR_MANDATORY_FIELD"), + label: t("HYP_ERR_MANDATORY_FIELD"),health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (2)
Line range hint
259-263: Update toast message for consistency.The error message here still uses
ERR_MANDATORY_FIELDwhile a similar validation inhandleNext()usesFOR_ERR_MANDATORY_FIELD. This should be updated for consistency.Apply this diff:
setShowToast({ key: "error", - label: t("ERR_MANDATORY_FIELD"), + label: t("FOR_ERR_MANDATORY_FIELD"), transitionTime: 3000, });
Line range hint
142-270: Consider extracting validation logic to reduce duplication.The validation logic for mandatory fields is duplicated between
handleNext()andhandleStepClick(). Consider extracting this into a shared function to improve maintainability and ensure consistent validation.Example refactor:
+ const validateMandatoryFields = (currentRuleConfigurations) => { + const existingFormulaOutputs = formulaConfigValues?.map((formula) => formula.output); + const visibleFormulas = currentRuleConfigurations.filter( + (item) => existingFormulaOutputs?.includes(item.output) && !deletedFormulas?.includes(item.output) + ); + + return !visibleFormulas.some((item) => { + const formulaConfig = formulaConfigValues.find((formula) => formula.output === item.output); + return !formulaConfig || !formulaConfig.input || !formulaConfig.operatorName || !formulaConfig.assumptionValue; + }); + }; const handleNext = () => { - if (formulaConfigValues.filter(row => row.category === currentCategory).every(row => { - return row.assumptionValue && row.input && row.output && row.operatorName - })) { + if (validateMandatoryFields(currentCategoryRuleConfigurations)) { // ... rest of the code } else { setShowToast({ key: "error", label: t("FOR_ERR_MANDATORY_FIELD"), transitionTime: 3000, }); return; } // ... rest of the code }; const handleStepClick = (step) => { const currentStepIndex = formulaInternalKey - 1; if (step === currentStepIndex + 1) { const currentRuleConfigurations = ruleConfigurationCategories[currentStepIndex]?.ruleConfigurations || []; - const existingFormulaOutputs = formulaConfigValues?.map((formula) => formula.output); - const visibleFormulas = currentRuleConfigurations?.filter( - (item) => existingFormulaOutputs?.includes(item.output) && !deletedFormulas?.includes(item.output) - ); - const hasEmptyFields = visibleFormulas.some((item) => { - const formulaConfig = formulaConfigValues.find((formula) => formula.output === item.output); - return !formulaConfig || !formulaConfig.input || !formulaConfig.operatorName || !formulaConfig.assumptionValue; - }); - if (hasEmptyFields) { + if (!validateMandatoryFields(currentRuleConfigurations)) { setShowToast({ key: "error", label: t("FOR_ERR_MANDATORY_FIELD"), transitionTime: 3000, }); return; } // ... rest of the code } // ... rest of the code };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js (1)
Pattern **/*.js: check
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js (1)
107-107: LGTM: Toast message differentiation implemented.
The change aligns with the PR objective of differentiating toast notifications for hypotheses by using the "HYP_" prefix.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (1)
149-149: LGTM! Toast message differentiation implemented correctly.
The change from ERR_MANDATORY_FIELD to FOR_ERR_MANDATORY_FIELD aligns with the PR objective of differentiating toast notifications for formulas.
Let's verify the translation key exists and is consistent:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (4)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js (1)
Line range hint
47-55: Consider using a more specific error message for assumption deletion.While the current error handling is good, the message "ERR_ATLEAST_ONE_MANDATORY_FIELD" could be more specific to the assumption context.
Consider creating a new translation key specific to assumptions:
setShowToast({ key: "error", - label: t("ERR_ATLEAST_ONE_MANDATORY_FIELD"), + label: t("ERR_ATLEAST_ONE_ASSUMPTION_REQUIRED"), transitionTime: 3000, });🧰 Tools
🪛 Biome
[error] 173-208: 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] 178-178: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 179-179: 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/microplan/src/components/FormulaConfiguration.js (3)
Line range hint
39-107: Consider improving state management in deletion logicThe current implementation of
handleConfirmDeletehas several areas that could be improved:
- Multiple state updates could lead to race conditions
- Complex nested state updates make the code harder to maintain
- Missing error handling for state updates
Consider refactoring the deletion logic using a more functional approach:
const handleConfirmDelete = () => { if (formulaToDelete !== null) { - const deletedFormula = formulas.find(operation => operation.output === formulaToDelete.output) + try { + const deletedFormula = formulas.find(operation => operation.output === formulaToDelete.output); + + // Batch all state updates + const updatedState = { + formulas: formulas.filter(operation => operation.output !== formulaToDelete.output), + deletedCategories: { + ...deletedFormulaCategories.current, + [category]: [...(deletedFormulaCategories.current[category] || []), deletedFormula.output] + }, + formulaConfigValues: formulaConfigValues.map(formula => ({ + ...formula, + input: formula.input === deletedFormula.output ? "" : formula.input, + assumptionValue: formula.assumptionValue === deletedFormula.output ? "" : formula.assumptionValue + })) + }; - //matching output for deleting a formula - const updatedFormulas = formulas.filter(operation => { - if (operation.output === formulaToDelete.output) { - return false - } - return true - }) + // Update all states at once + setFormulas(updatedState.formulas); + deletedFormulaCategories.current = updatedState.deletedCategories; + setDeletedFormulas(prev => [...prev, deletedFormula.output]); + setFormulaConfigValues(updatedState.formulaConfigValues); + } catch (error) { + setShowToast({ + key: "error", + label: t("ERR_FORMULA_DELETE_FAILED"), + transitionTime: 3000, + }); + } + setFormulaToDelete(null); + setShowPopUp(false); } };🧰 Tools
🪛 Biome
[error] 331-360: 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] 336-336: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 337-337: 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)
Line range hint
108-196: Clean up commented code and improve formula addition logic
- Remove commented-out code blocks as they add noise
- The formula ordering logic is complex and could be simplified
- Consider adding validation for duplicate formulas
Consider this cleaner implementation:
const addNewFormula = () => { if (!selectedDeletedFormula) return; const formulaToAdd = selectedDeletedFormula.code; + // Early validation + if (formulas.some(formula => formula.output === formulaToAdd)) { + setShowToast({ + key: "error", + label: t("ERR_FORMULA_ALREADY_EXISTS"), + transitionTime: 3000, + }); + return; + } const newFormula = { source: "MDMS", output: formulaToAdd, input: "", operatorName: "", assumptionValue: "", category, showOnEstimationDashboard: true }; + // Find insertion index based on category grouping + const categoryFormulas = formulaConfigValues.filter(f => f.category === category); + const insertIndex = categoryFormulas.length ? + formulaConfigValues.indexOf(categoryFormulas[categoryFormulas.length - 1]) + 1 : + formulaConfigValues.length; setFormulas(prev => [...prev, newFormula]); setFormulaConfigValues(prev => [ ...prev.slice(0, insertIndex), newFormula, ...prev.slice(insertIndex) ]); setSelectedDeletedFormula(null); setFormulasPopUp(false); };🧰 Tools
🪛 Biome
[error] 331-360: 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] 336-336: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 337-337: 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)
Line range hint
1-424: Consider implementing a Formula Management HookThe component has grown complex with multiple state management responsibilities. Consider extracting the formula management logic into a custom hook (e.g.,
useFormulaManagement) to:
- Separate concerns between UI and business logic
- Make the state management more testable
- Reduce component complexity
- Enable reuse of formula management logic
Would you like assistance in creating this custom hook?
🧰 Tools
🪛 Biome
[error] 331-360: 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] 336-336: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 337-337: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js (1)
Pattern **/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
[error] 336-336: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 337-337: 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/microplan/src/components/Hypothesis.js
[error] 178-178: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 179-179: 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)
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js (1)
176-180: LGTM! Good differentiation of confirmation messages.
The addition of the "HYP_" prefix helps distinguish hypothesis-related messages from other confirmations, improving context clarity for users.
Let's verify the translation keys are properly defined:
🧰 Tools
🪛 Biome
[error] 178-178: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 179-179: 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/microplan/src/components/FormulaConfiguration.js (1)
334-334: LGTM: Toast message prefixing aligns with PR objective
The addition of "FOR_" prefix to confirmation messages helps differentiate formula-related notifications from other types, which aligns well with the PR objective.
Also applies to: 338-338
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js
Show resolved
Hide resolved
...-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
Show resolved
Hide resolved
Toast for formula and assumptions kept different
Summary by CodeRabbit
New Features
Bug Fixes