DEMO UI Bug fixes #1665
DEMO UI Bug fixes #1665
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple components in the microplan module. Key changes include disabling the step click functionality in various Changes
Possibly related PRs
Suggested reviewers
Poem
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: 7
🧹 Outside diff range comments (23)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UserAccessMgmtTableWrapper.js (6)
Line range hint
41-76: Consider extracting Wrapper component to a separate file.The
Wrappercomponent is a self-contained popup component that could be moved to its own file to improve code organization and maintainability.
Line range hint
120-124: Prevent potential memory leaks in useEffect.The current implementation might cause memory leaks if the component unmounts during data fetching. Consider adding cleanup logic.
useEffect(() => { + let isSubscribed = true; refetchPlanSearch(); + return () => { + isSubscribed = false; + }; }, [currentPage, rowsPerPage]);
Line range hint
126-166: Consider memoizing the columns configuration.The columns array is recreated on every render. Consider using
useMemoto optimize performance, especially since it contains translation functions.- const columns = [ + const columns = useMemo(() => [ // ... existing columns config ... - ]; + ], [t]); // Add t as dependency since it's used in column definitions
Line range hint
168-170: Extract pagination configuration to constants.The pagination values (5, 10, 15, 20) are hardcoded. Consider extracting these to configuration constants for better maintainability.
+ const ROWS_PER_PAGE_OPTIONS = [5, 10, 15, 20]; + const DEFAULT_ROWS_PER_PAGE = 5; - const [rowsPerPage] = useState(5); + const [rowsPerPage] = useState(DEFAULT_ROWS_PER_PAGE); // ... in DataTable props ... - paginationRowsPerPageOptions={[5, 10, 15, 20]} + paginationRowsPerPageOptions={ROWS_PER_PAGE_OPTIONS}
Line range hint
172-207: Simplify jurisdiction cell rendering logic.The jurisdiction cell renderer contains complex nested conditionals and ternary operations. Consider extracting this logic to a separate component for better readability and maintainability.
Consider creating a separate
JurisdictionCellcomponent to handle this complex rendering logic:const JurisdictionCell = ({ jurisdiction, onViewMore }) => { const { t } = useTranslation(); const visibleJurisdictions = jurisdiction?.slice(0, 2); const remainingCount = jurisdiction?.length - 2; return ( <div className="digit-tag-container userAccessCell"> {visibleJurisdictions?.map((value, index) => ( <JurisdictionChip key={index} value={value} /> ))} {remainingCount > 0 && ( <ViewMoreButton count={remainingCount} onClick={onViewMore} /> )} </div> ); };
Based on the gathered evidence:
- The hook
usePlanSearchEmployeeWithTaggingis implemented usingreact-querywhich provides error states- Multiple components in the codebase consistently handle errors from similar hooks (as seen in FormulaConfigWrapper, HypothesisWrapper, RoleTableComposer, etc.)
- The pattern
{ isLoading, data, error, refetch }is consistently used across the codebaseAdd error handling to match codebase patterns
- const { isLoading, data: planAssignmentData, refetch: refetchPlanSearch} = Digit.Hooks.microplanv1.usePlanSearchEmployeeWithTagging({ + const { isLoading, data: planAssignmentData, error, refetch: refetchPlanSearch} = Digit.Hooks.microplanv1.usePlanSearchEmployeeWithTagging({ // ... existing config ... }); + if (error) { + return ( + <Card> + <div className="error-message"> + {t("MICROPLAN_ERROR_FETCHING_DATA")} + </div> + </Card> + ); + }🔗 Analysis chain
Line range hint
92-117: Add error handling to the data fetching hook.The
usePlanSearchEmployeeWithTagginghook implementation should include error handling to provide feedback to users when data fetching fails.const { isLoading, data: planAssignmentData, refetch: refetchPlanSearch} = Digit.Hooks.microplanv1.usePlanSearchEmployeeWithTagging({ + const { isLoading, data: planAssignmentData, error, refetch: refetchPlanSearch} = Digit.Hooks.microplanv1.usePlanSearchEmployeeWithTagging({ // ... existing config ... }); + if (error) { + return ( + <Card> + <div className="error-message"> + {t("MICROPLAN_ERROR_FETCHING_DATA")} + </div> + </Card> + ); + }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if error handling exists in other similar components rg -A 5 "usePlanSearchEmployeeWithTagging.*error" .Length of output: 53
Script:
#!/bin/bash # Let's check for error handling patterns in similar hooks or components rg -A 5 "Digit\.Hooks.*error" .Length of output: 33
Script:
#!/bin/bash # Check the hook implementation itself fd "usePlanSearchEmployeeWithTagging" --exec cat {}Length of output: 575
Script:
#!/bin/bash # Look for error handling patterns in similar table components rg -A 5 "isLoading.*error.*refetch" .Length of output: 13921
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1)
Line range hint
1-424: Consider architectural improvements for better maintainability.While the current implementation works, the component could benefit from these architectural improvements:
- Consider using
useReducerto simplify state management instead of multipleuseStatehooks- Extract navigation logic into a custom hook (e.g.,
useMicroplanNavigation)- Move form submission logic to a separate service
- Consider breaking down the component into smaller, more focused components
These changes would improve maintainability and testability while reducing the complexity of the main component.
Would you like me to provide example implementations for any of these suggestions?
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js (3)
Line range hint
152-207: Potential infinite loop in useEffect without dependencies.The useEffect hook without dependencies will run after every render, potentially causing infinite re-renders and performance issues.
Update the useEffect to include proper dependencies:
- useEffect(() => { - if (executionCount < 5) { - onSelect(customProps.name, {assumptionValues}) - setExecutionCount((prevCount) => prevCount + 1); - } - }); + useEffect(() => { + if (executionCount < 5) { + onSelect(customProps.name, {assumptionValues}) + setExecutionCount((prevCount) => prevCount + 1); + } + }, [executionCount, customProps.name, assumptionValues]);
Line range hint
73-117: Consolidate duplicate validation logic.The validation logic for empty fields is duplicated between
handleNextandhandleStepClick. This violates the DRY principle and makes maintenance harder.Extract the validation logic into a separate function:
+ const validateCurrentStep = (currentStepIndex) => { + const currentAssumptions = assumptionCategories[currentStepIndex]?.assumptions || []; + const existingAssumptionKeys = assumptionValues.map(assumption => assumption.key); + const visibleAssumptions = currentAssumptions.filter(item => + existingAssumptionKeys?.includes(item) && !deletedAssumptions?.includes(item) + ); + + const hasEmptyFields = visibleAssumptions.some(item => { + const value = assumptionValues.find(assumption => assumption.key === item)?.value; + return !value; + }); + + if (hasEmptyFields) { + setShowToast({ + key: "error", + label: t("ERR_MANDATORY_FIELD"), + transitionTime: 3000, + }); + return false; + } + return true; + }; const handleNext = () => { - const currentAssumptions = assumptionCategories[internalKey - 1]?.assumptions || []; - // ... existing validation logic ... + if (!validateCurrentStep(internalKey - 1)) return; // Continue with API call and navigation }; const handleStepClick = (step) => { const currentStepIndex = internalKey - 1; if (step === currentStepIndex + 1) { - // ... existing validation logic ... + if (!validateCurrentStep(currentStepIndex)) return; setInternalKey(step + 1); } // ... rest of the function };Also applies to: 119-157
Line range hint
138-157: Consider using a more robust state management solution.The component uses multiple useEffect hooks with similar dependencies to manage state updates and side effects. This makes the component's behavior hard to predict and maintain.
Consider:
- Using a reducer to manage related state updates
- Implementing a custom hook to encapsulate the assumption management logic
- Using a state management library like Redux or Zustand for complex state interactions
Example custom hook:
const useAssumptionManager = (initialAssumptions, onSelect) => { const [assumptionValues, setAssumptionValues] = useState(initialAssumptions); const [executionCount, setExecutionCount] = useState(0); useEffect(() => { if (executionCount < 5) { onSelect(assumptionValues); setExecutionCount(prev => prev + 1); } }, [executionCount, assumptionValues, onSelect]); return { assumptionValues, setAssumptionValues, executionCount }; };health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UICustomizations.js (3)
Line range hint
401-402: Move hook outside configuration functionUsing React hooks inside configuration functions is an anti-pattern that could lead to runtime errors. The
usePlanSearchEmployeeWithTagginghook should be called in the component and its result passed to the configuration.Suggested approach:
- Move the hook to the component using this configuration
- Pass the jurisdiction data through the
additionalDetailsparameter- const { - isLoading: isPlanEmpSearchLoading, - data: planEmployee, - error: planEmployeeError, - refetch: refetchPlanEmployee, - } = Digit.Hooks.microplanv1.usePlanSearchEmployeeWithTagging({...}); + const jurisdiction = additionalDetails?.planEmployee?.jurisdiction; data.body.PlanFacilitySearchCriteria = { ...data.body.PlanFacilitySearchCriteria, planConfigurationId: url?.microplanId, - jurisdiction:planEmployee?.planData?.[0]?.jurisdiction + jurisdiction: jurisdiction };
Line range hint
18-33: Move utility function to separate fileThe
cleanObjectutility function is used across multiple configurations. Consider moving it to a dedicated utilities file to promote reusability and maintainability.Create a new file
utils/objectUtils.js:export const cleanObject = (obj) => { // ... existing implementation };Then import it where needed:
import { cleanObject } from '../utils/objectUtils';
Line range hint
472-476: Add default case to switch statementThe switch statement in additionalCustomizations is missing a default case. This could lead to undefined behavior for unknown keys.
switch (key) { case "MICROPLAN_FACILITY_ASSIGNED_VILLAGES": const assignedVillages = row?.additionalDetails?.assignedVillages; return assignedVillages ? assignedVillages.length : null; case "MICROPLAN_FACILITY_ACTION": return ( // ... existing implementation ); default: - return null; + return t("ES_COMMON_NA"); }health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (3)
Line range hint
502-509: Fix potential infinite loop and missing dependenciesThe useEffect hook without dependencies will run on every render, and the execution count limit seems like a workaround for an underlying issue.
-useEffect(() => { - if (executionCount < 5) { - onSelect(customProps.name, {formulaConfigValues}) - setExecutionCount((prevCount) => prevCount + 1); - } -}); +useEffect(() => { + onSelect(customProps.name, {formulaConfigValues}) +}, [customProps.name, formulaConfigValues]);
Line range hint
267-297: Enhance error handling and validationThe current error handling relies heavily on toast notifications and lacks proper error boundaries. Consider:
- Adding error boundaries to handle component crashes
- Centralizing validation logic
- Adding proper error handling for external calls
Example error boundary implementation:
class FormulaErrorBoundary extends React.Component { state = { hasError: false }; static getDerivedStateFromError(error) { return { hasError: true }; } componentDidCatch(error, errorInfo) { // Log error to monitoring service console.error(error, errorInfo); } render() { if (this.state.hasError) { return <h1>{t("SOMETHING_WENT_WRONG")}</h1>; } return this.props.children; } }Also applies to: 535-548
Line range hint
449-484: Optimize performance with memoizationThe filtering operations for
filteredRuleConfigureInputsandfilteredRuleConfigureOutputsare complex and could benefit from memoization. Additionally, the component handles too many responsibilities.Consider these improvements:
- Memoize filtering operations:
const filteredRuleConfigureInputs = useMemo(() => { return state.RuleConfigureInputs.filter((item) => { const isHouseToHouseOrFixedPost = resourceDistributionStrategyCode === "HOUSE_TO_HOUSE" || resourceDistributionStrategyCode === "FIXED_POST"; // ... rest of the filtering logic }); }, [state.RuleConfigureInputs, resourceDistributionStrategyCode, campaignType, assumptionsFormValues]);
- Split the component into smaller, focused components:
- FormulaStepperNav
- FormulaValidation
- FormulaConfiguration (already exists)
Also applies to: 485-503
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PlanInbox.js (2)
Line range hint
89-148: Add error handling for data fetching hooks.The component uses several data fetching hooks but doesn't handle their error states. Consider implementing error boundaries or error messages for a better user experience.
Example implementation for the plan search hook:
if (isPlanWithCensusLoading || isPlanEmpSearchLoading || isLoadingCampaignObject || isWorkflowLoading) { return <Loader />; } + if (planWithCensusError || planWithCensusError || errorCampaign || planEmployeeError) { + return <Card className="error-card"> + {t("MICROPLAN_ERROR_FETCHING_DATA")} + </Card>; + }
Line range hint
365-397: Consider abstracting workflow update logic.The workflow update logic in
updateWorkflowForSelectedRowsandupdateWorkflowForFooterActioncould be abstracted into a utility function to improve code reusability and maintainability.Consider creating a utility function:
const updateWorkflow = (data, action, type = 'rows') => { if (type === 'rows') { return data.map(({ original }) => ({ ...original, workflow: { ...original.workflow, action, }, })); } return { ...data, workflow: { ...data?.workflow, action, }, }; }; // Usage: const updateWorkflowForSelectedRows = () => updateWorkflow(selectedRows, workFlowPopUp, 'rows'); const updateWorkflowForFooterAction = () => updateWorkflow(planObject, 'APPROVE_ESTIMATIONS', 'config');health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (5)
Line range hint
32-38: Potential memory leak in useEffect cleanup.The timeout for toast messages is set but not properly cleaned up in all cases.
Add cleanup function to prevent memory leaks:
useEffect(() => { if (showToast) { const t = setTimeout(closeToast, 50000); return () => clearTimeout(t); } + return () => {}; // Add empty cleanup function for when showToast is false }, [showToast]);
Line range hint
261-324: Improve error handling in file upload process.The error handling in
onBulkUploadSubmitcould be more robust and user-friendly.Consider implementing a more comprehensive error handling approach:
const onBulkUploadSubmit = async (file) => { try { if (file.length > 1) { setShowToast({ key: "error", label: t("HCM_ERROR_MORE_THAN_ONE_FILE") }); return; } + // Add file type validation + const allowedTypes = ['application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', 'application/vnd.ms-excel']; + if (!allowedTypes.includes(file[0].type)) { + setShowToast({ key: "error", label: t("HCM_ERROR_INVALID_FILE_TYPE") }); + return; + } setFileName(file?.[0]?.name); const module = "HCM-ADMIN-CONSOLE-CLIENT"; // Try uploading the file const { data: { files: fileStoreIds } = {} } = await Digit.UploadServices.MultipleFilesStorage(module, file, tenantId); if (!fileStoreIds || fileStoreIds.length === 0) { + setIsError(true); throw new Error(t("HCM_ERROR_FILE_UPLOAD_FAILED")); }
Line range hint
371-436: Optimize validation process with debouncing.The validation process in
useEffectcould benefit from debouncing to prevent excessive API calls.Implement debouncing for the validation process:
+import { debounce } from 'lodash'; +const debouncedValidation = debounce(async (uploadedFile, boundaryHierarchy, type, tenantId, id, baseTimeOut) => { + // Existing validation logic +}, 500); useEffect(() => { const fetchData = async () => { if (!errorsType[type] && uploadedFile?.length > 0 && !isSuccess) { setIsValidation(true); setLoader(true); - try { - const temp = await Digit.Hooks.campaign.useResourceData( + debouncedValidation( uploadedFile, boundaryHierarchy, type, tenantId, id, baseTimeOut?.["HCM-ADMIN-CONSOLE"], { source : "microplan" } );
Line range hint
437-492: Template download function needs retry mechanism.The template download functionality lacks a retry mechanism for failed downloads.
Implement a retry mechanism for template downloads:
+const MAX_RETRIES = 3; +const RETRY_DELAY = 2000; +const downloadTemplateWithRetry = async (retryCount = 0) => { + try { + await downloadTemplate(); + } catch (error) { + if (retryCount < MAX_RETRIES) { + setTimeout(() => { + downloadTemplateWithRetry(retryCount + 1); + }, RETRY_DELAY); + } else { + setDownloadError(true); + setShowToast({ key: "error", label: t("ERROR_MAX_RETRIES_REACHED") }); + } + } +};
Line range hint
493-683: Improve component performance with useMemo and useCallback.Several functions and computed values could benefit from memoization.
Implement memoization for better performance:
+const memoizedTemplate = useMemo(() => ({ + url: "/project-factory/v1/data/_download", + params: { + tenantId, + type, + hierarchyType: boundaryHierarchy, + id: type === "boundary" ? params?.boundaryId : type === "facilityWithBoundary" ? params?.facilityId : params?.userId, + }, +}), [tenantId, type, boundaryHierarchy, params]); +const handleFileDelete = useCallback((file, index) => { + setUploadedFile((prev) => prev.filter((i) => i.id !== file.id)); + setIsError(false); + setProcessedFile([]); + setSheetErrors(0); + setShowPreview(false); + setIsSuccess(false); + setIsValidation(false); + setApiError(null); + setErrorsType({}); + setShowToast(null); +}, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (8)
- 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)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UserAccessMgmtTableWrapper.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UserAccessWrapper.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UICustomizations.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PlanInbox.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
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/HypothesisWrapper.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UserAccessMgmtTableWrapper.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UserAccessWrapper.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/pages/employee/PlanInbox.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1)
Pattern
**/*.js: check
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js (1)
310-310: 🛠️ Refactor suggestionVerify the impact of disabling step navigation.
While disabling step navigation via
onStepClick={() => null}prevents users from jumping between steps, thehandleStepClickfunction is still maintained but unused. This could lead to confusion for future maintenance.Consider either:
- Removing the unused
handleStepClickfunction if step navigation should be permanently disabled, or- Keeping the click navigation with proper validation by restoring
onStepClick={handleStepClick}health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (1)
406-408: 🧹 Nitpick (assertive)Consider the implications of disabling step navigation
While disabling step navigation by setting
onStepClick={() => null}prevents users from jumping between steps, it might negatively impact user experience. The existing validation logic inhandleStepClicksuggests that controlled navigation between steps was intentionally designed.Consider these alternatives:
- Keep the validation logic but only allow navigation to completed steps
- Show a modal explaining why certain steps are not accessible
- Visually indicate which steps are available for navigation
If step navigation must be disabled, the unused
handleStepClickfunction should be removed to maintain code cleanliness.
...b/micro-ui-internals/packages/modules/microplan/src/components/UserAccessMgmtTableWrapper.js
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js
Show resolved
Hide resolved
...h/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UICustomizations.js
Show resolved
Hide resolved
...h/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PlanInbox.js
Show resolved
Hide resolved
...h/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PlanInbox.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
...cro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UserAccessWrapper.js
Show resolved
Hide resolved
...h/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UICustomizations.js
Show resolved
Hide resolved
Ramkrishna-egov
dismissed stale reviews from coderabbitai[bot] and nabeelmd-eGov
via
0d2643c
Choose the appropriate template for your PR:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores