fixes #1869
fixes #1869
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to three main components: 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: 3
🧹 Outside diff range comments (8)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js (2)
Line range hint
126-156: Remove commented out configuration code.There's a large block of commented out configuration code that should be removed if it's no longer needed. If this is temporary or for reference, consider moving it to documentation.
Line range hint
51-229: Consider splitting the App component for better maintainability.The App component handles multiple responsibilities (session management, data fetching, routing, UI). Consider splitting it into smaller, focused components:
- A SessionManager component for handling session-related logic
- A DataProvider component for MDMS data fetching
- A Router component for route definitions
- The main App component orchestrating these components
This would improve:
- Code organization and maintainability
- Testing capabilities
- Reusability of individual components
🧰 Tools
🪛 Biome
[error] 207-207: 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] 208-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)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js (3)
Line range hint
1-585: Consider adopting TypeScript for better type safetyThe component handles complex state management and data structures. Consider adopting TypeScript to:
- Add proper type definitions for formula objects and their properties
- Prevent potential runtime errors from incorrect property access
- Improve code maintainability and IDE support
Example type definitions:
interface Formula { source: 'CUSTOM' | 'MDMS'; output: string; input: string; operatorName: string; assumptionValue: string; category: string; showOnEstimationDashboard: boolean; } interface FormulaConfigurationProps { onSelect: (formula: Formula) => void; category: string; customProps: { filteredInputs: string[]; filteredOutputs: string[]; operators: Array<{operatorCode: string, operatorName: string}>; assumptions: string[]; }; formulas: Formula[]; setShowToast: (toast: {key: string, label: string, transitionTime: number}) => void; allMdmsFormulasForThisCategory: string[]; }
Line range hint
39-47: Centralize error handling and validation logicThe error handling for formula deletion could be improved by:
- Centralizing validation logic in a separate utility function
- Adding more specific error messages for different validation scenarios
- Implementing a consistent error handling pattern
Example refactor:
+ const validateFormulaOperation = (operation, formula) => { + const errors = []; + if (operation === 'delete') { + if (formulas?.length === 1 && category !== "CAMPAIGN_VEHICLES") { + errors.push("ERR_ATLEAST_ONE_MANDATORY_FORMULA"); + } + } + return errors; + } const handleDeleteClick = (index, formula) => { - if (formulas?.length === 1 && category !== "CAMPAIGN_VEHICLES") { - setShowToast({ - key: "error", - label: t("ERR_ATLEAST_ONE_MANDATORY_FORMULA"), - transitionTime: 3000, - }); - return; - } + const errors = validateFormulaOperation('delete', formula); + if (errors.length > 0) { + setShowToast({ + key: "error", + label: t(errors[0]), + transitionTime: 3000, + }); + return; + } setFormulaToDelete(formula); setShowPopUp(formula?.source || true); };
Line range hint
283-321: Optimize performance with memoizationConsider optimizing the component's performance by:
- Memoizing dropdown options calculations
- Using
useMemofor complex computations- Implementing
React.memofor child componentsExample optimization:
+ const MemoizedCard = React.memo(Card); + const dropdownOptions = useMemo(() => { + return [ + ...filteredInputs.map((input) => ({ code: input, label: input })), + ...previousOutputs.map((output) => ({ code: output, label: output })), + ...assumptions.map((assumptionValue) => ({ + code: assumptionValue, + label: assumptionValue, + })), + ]; + }, [filteredInputs, previousOutputs, assumptions]);health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (3)
Line range hint
249-284: Improve validation logic in handleNext function.The current validation has several issues:
- The SUBSTRACTION operator validation returns early, but the empty fields validation proceeds with the update
- No validation for null/undefined values
- Generic error messages that don't guide users to specific fields
Consider refactoring the validation logic:
const handleNext = () => { + const currentFormulas = formulaConfigValues.filter((row) => row.category === currentCategory); + + // Validate for null/undefined values first + const hasInvalidValues = currentFormulas.some( + (row) => row.assumptionValue == null || row.input == null || row.output == null || row.operatorName == null + ); + + if (hasInvalidValues) { + setShowToast({ + key: "error", + label: t("ERR_INVALID_VALUES"), + transitionTime: 3000, + }); + return; + } + // Check SUBSTRACTION operator - if (formulaConfigValues.some((i) => i.operatorName === "SUBSTRACTION" && i.input === i.assumptionValue)) { + const hasInvalidSubstraction = currentFormulas.some( + (i) => i.operatorName === "SUBSTRACTION" && i.input === i.assumptionValue + ); + + if (hasInvalidSubstraction) { setShowToast({ key: "error", label: t("ERR_MANDATORY_FIELD_SAME_OPERAND"), transitionTime: 3000, }); return; - } else if ( - formulaConfigValues - .filter((row) => row.category === currentCategory) - .every((row) => { - return row.assumptionValue && row.input && row.output && row.operatorName; - }) - ) { - //will do this on onSuccess - } else { + } + + // Check for empty required fields + const hasEmptyFields = currentFormulas.some( + (row) => !row.assumptionValue || !row.input || !row.output || !row.operatorName + ); + + if (hasEmptyFields) { setShowToast({ key: "error", - label: t("ERR_MANDATORY_FIELD"), + label: t("ERR_MISSING_REQUIRED_FIELDS"), transitionTime: 3000, }); return; }🧰 Tools
🪛 Biome
[error] 536-536: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
Line range hint
411-416: Fix potential infinite render and race condition in useEffect.This effect has several issues:
- Missing dependency array causes it to run on every render
- Arbitrary execution count limit
- Potential race condition with the other effect that calls
onSelectConsider removing this effect and merging its logic with the other effect that handles the same data:
-useEffect(() => { - if (executionCount < 5) { - onSelect(customProps.name, { formulaConfigValues }); - setExecutionCount((prevCount) => prevCount + 1); - } -}); useEffect(() => { onSelect(customProps.name, { formulaConfigValues }); -}, [formulaConfigValues, formulaInternalKey]); +}, [formulaConfigValues, formulaInternalKey, customProps.name]);🧰 Tools
🪛 Biome
[error] 536-536: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
Line range hint
285-301: Enhance error handling for API calls.The current error handling for updateResources is basic and could be improved to handle different types of failures and provide better user feedback.
Consider implementing a more robust error handling strategy:
updateResources( { config: { name: "SUB_FORMULA", }, formulasToUpdate, state, }, { onSuccess: (data) => { setManualLoader(false); if (formulaInternalKey < ruleConfigurationCategories?.length) { setFormulaInternalKey((prevKey) => prevKey + 1); } refetchPlan(); }, onError: (error, variables) => { setManualLoader(false); - console.error(error); - setShowToast({ key: "error", label: error?.message ? error.message : t("FAILED_TO_UPDATE_RESOURCE") }); + const isNetworkError = !error.response; + const isServerError = error.response?.status >= 500; + const isValidationError = error.response?.status === 400; + + console.error('Resource update failed:', { + error, + variables, + type: isNetworkError ? 'network' : isServerError ? 'server' : 'validation' + }); + + setShowToast({ + key: "error", + label: isNetworkError + ? t("ERR_NETWORK_ISSUE") + : isServerError + ? t("ERR_SERVER_ISSUE") + : error?.message || t("FAILED_TO_UPDATE_RESOURCE"), + transitionTime: 3000 + }); + + // Retry for network or server errors + if ((isNetworkError || isServerError) && variables.retryCount < 3) { + setTimeout(() => { + updateResources({ + ...variables, + retryCount: (variables.retryCount || 0) + 1 + }); + }, 1000 * Math.pow(2, variables.retryCount || 0)); + } }, } );🧰 Tools
🪛 Biome
[error] 536-536: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
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/FormulaConfiguration.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js(7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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/FormulaConfiguration.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js (1)
Pattern **/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js
[error] 207-207: 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] 208-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)
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js (1)
274-277: LGTM! Improved text localization and descriptions
The changes improve the user experience by:
- Using more specific translation keys with the "FORMULA_" prefix
- Adding context-specific descriptions based on the category
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (1)
535-535: 🛠️ Refactor suggestion
Verify translation key consistency for category prop.
The category prop is passed directly without the "FORMULA_" prefix, but the Stepper component and other parts of the code expect translation keys with this prefix. This inconsistency might cause translation issues.
Consider maintaining consistency by either:
- Adding the prefix in FormulaConfiguration:
-category={ruleConfigurationCategories[formulaInternalKey - 1]?.category}
+category={`FORMULA_${ruleConfigurationCategories[formulaInternalKey - 1]?.category}`}- Or removing the prefix from Stepper and updating translation files accordingly.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js
Show resolved
Hide resolved
...-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
Outdated
Show resolved
Hide resolved
…lan/src/components/FormulaConfiguration.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (4)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js (4)
Line range hint
32-42: Consider adding loading state during formula operationsThe formula deletion and addition operations modify multiple state variables (
formulas,formulaConfigValues,deletedFormulas). Consider adding a loading state to prevent multiple operations from being triggered simultaneously and to provide better user feedback.const FormulaConfiguration = ({ onSelect, category, customProps, formulas: initialFormulas, setShowToast, allMdmsFormulasForThisCategory }) => { const { t } = useTranslation(); + const [isLoading, setIsLoading] = useState(false); const [showPopUP, setShowPopUp] = useState(false); const [formulasPopUP, setFormulasPopUp] = useState(false); // ... rest of the code
Line range hint
155-220: Consider extracting formula insertion logic into a utility functionThe formula insertion logic is duplicated in multiple places with similar patterns. This could be extracted into a utility function to improve maintainability and reduce code duplication.
+ const insertFormulaInOrder = (prevValues, newFormula, category) => { + const lastIndexInCategory = [...prevValues].reverse().findIndex((item) => item.category === category); + const insertIndex = lastIndexInCategory === -1 ? prevValues.length : prevValues.length - lastIndexInCategory; + return [...prevValues.slice(0, insertIndex), newFormula, ...prevValues.slice(insertIndex)]; + }; const addNewFormula = () => { if (selectedDeletedFormula?.code === "NEW_FORMULA") { // ... existing code ... setFormulaConfigValues((prevValues) => { const newFormula = { source: "CUSTOM", output: formulaToAdd?.name, input: "", operatorName: "", assumptionValue: "", category: category, showOnEstimationDashboard: true, }; - const lastIndexInCategory = [...prevValues].reverse().findIndex((item) => item.category === category); - const insertIndex = lastIndexInCategory === -1 ? prevValues.length : prevValues.length - lastIndexInCategory; - const newValues = [...prevValues.slice(0, insertIndex), newFormula, ...prevValues.slice(insertIndex)]; - return newValues; + return insertFormulaInOrder(prevValues, newFormula, category); }); } };
Line range hint
44-82: Add error handling for formula deletion edge casesThe formula deletion logic should handle edge cases more gracefully, such as when
formulaToDeleteis undefined or when the formula is referenced by other formulas.const handleDeleteClick = (index, formula) => { + if (!formula) { + setShowToast({ + key: "error", + label: t("ERR_INVALID_FORMULA"), + transitionTime: 3000, + }); + return; + } + // Check if formula is referenced by other formulas + const isReferenced = formulaConfigValues.some( + (f) => f.input === formula.output || f.assumptionValue === formula.output + ); + + if (isReferenced) { + setShowToast({ + key: "error", + label: t("ERR_FORMULA_IN_USE"), + transitionTime: 3000, + }); + return; + } if (formulas?.length === 1 && category !== "CAMPAIGN_VEHICLES") { // ... existing code ... } setFormulaToDelete(formula); setShowPopUp(formula?.source || true); };
Based on our investigation, I can now provide the final response:
Translation keys for formula messages need to be added to localization files
The code uses several translation keys (
FORMULA_HEADER_${category},FORMULA_VEHICLE_DESCRIPTION,FORMULA_CONFIGURATION_DESCRIPTION,FORMULA_MESSAGE_FOR_${formula.output},FOR_CONFIRM_TO_DELETE,FORMULA_PERMANENT_DELETE_CUSTOM,FOR_PERMANENT_DELETE,FORMULA_NAME) but we couldn't find these translations defined in any localization files. This will result in missing translations in the UI.
- Add these translation keys to the appropriate localization files
- Ensure translations are added for all supported languages
- Consider adding a default fallback text for these keys
🔗 Analysis chain
Line range hint
274-450: Verify translation keys exist for all formula messagesThe component uses several translation keys for formula-related messages. Ensure all these keys are defined in the translation files to avoid missing translations.
Let me gather more specific information about the translation keys by searching in the localization files.
Let me try a different approach to find the translation files and their content.
Let me check one more location where translations might be stored - the root public directory of the micro-ui, as it's common to store translations there.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for missing translation keys in the codebase # Search for translation keys used in the file echo "Translation keys used in FormulaConfiguration.js:" rg -o 't\(["`]([^"`]+)["`]\)' | sed 's/.*t(["`]\([^"`]*\)["`]).*/\1/' > /tmp/used_keys.txt # Search for translation key definitions echo "Searching for translation key definitions:" rg -l '(FORMULA_HEADER|FORMULA_VEHICLE_DESCRIPTION|FORMULA_CONFIGURATION_DESCRIPTION|FORMULA_MESSAGE_FOR|FOR_CONFIRM_TO_DELETE|FORMULA_PERMANENT_DELETE_CUSTOM|FOR_PERMANENT_DELETE)'Length of output: 564
Script:
#!/bin/bash # Search for translation files and their content echo "Searching for translation/localization files:" fd -e json -e js "translation|localization|i18n|locale" --type f echo -e "\nChecking content of translation files for formula-related keys:" rg -l "FORMULA_|FOR_" $(fd -e json -e js "translation|localization|i18n|locale" --type f) echo -e "\nShowing actual translations for formula keys:" rg "FORMULA_|FOR_" $(fd -e json -e js "translation|localization|i18n|locale" --type f)Length of output: 1060
Script:
#!/bin/bash # Search for translation files in the microplan module specifically echo "Searching for translation files in microplan module:" fd . health/micro-ui/web/micro-ui-internals/packages/modules/microplan -t f | grep -i "translation\|locale\|i18n" echo -e "\nSearching for any .json files that might contain translations:" fd . health/micro-ui/web/micro-ui-internals/packages/modules/microplan -e json -t f echo -e "\nChecking content of public/localization folder if it exists:" fd . health/micro-ui/web/micro-ui-internals/packages/modules/microplan/public -t f | grep -i "local" # Also check for hardcoded strings in the component echo -e "\nChecking FormulaConfiguration.js for hardcoded translation strings:" rg "FORMULA_|FOR_" health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.jsLength of output: 1920
Script:
#!/bin/bash # Check for translations in the root public directory echo "Searching for localization files in micro-ui public directory:" fd . health/micro-ui/web/public -t f | grep -i "local" echo -e "\nChecking content of any found localization files for formula keys:" rg "FORMULA_|FOR_" health/micro-ui/web/public/localization/en_IN.json || true rg "FORMULA_|FOR_" health/micro-ui/web/public/localization/hi_IN.json || true # Also check the modules directory for any localization echo -e "\nChecking modules directory for localization:" fd . health/micro-ui/web/micro-ui-internals/packages/modules -t f | grep -i "local"Length of output: 1096
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js (1)
Pattern **/*.js: check
...-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
Show resolved
Hide resolved
Choose the appropriate template for your PR:
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation