-
Notifications
You must be signed in to change notification settings - Fork 22
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
draft issue fix, table reload fix, table no data fix #1664
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to three components: Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)
🔇 Additional comments (3)
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 (
|
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.
Actionable comments posted: 6
🧹 Outside diff range comments (3)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (2)
Line range hint
156-167
: Enhance error handling in form validationThe form validation logic could benefit from more detailed error messages and structured error handling. Currently, it only shows a generic toast message.
Consider enhancing the validation:
const toastObject = Digit.Utils.microplanv1.formValidator(formData?.[currentConfBody?.key], currentConfBody?.key, state); if (toastObject) { + const errorDetails = { + field: currentConfBody?.key, + formData: formData?.[currentConfBody?.key], + message: toastObject.label + }; + console.debug('Form validation failed:', errorDetails); setShowToast(toastObject); + setIsSubmitting(false); // Reset submission state on validation failure return; }
Line range hint
211-234
: Consolidate navigation event handlersThe component has multiple event listeners for navigation (moveToPrevious, revertToPreviousScreenFromFormula) with similar functionality. This could be consolidated into a single, more maintainable navigation handler.
Consider consolidating the navigation logic:
+const handleNavigation = (type) => { + setCurrentStep((prev) => prev - 1); + setCurrentKey((prev) => prev - 1); +}; + useEffect(() => { - window.addEventListener("moveToPrevious", moveToPreviousStep); - return () => { - window.removeEventListener("moveToPrevious", moveToPreviousStep); - }; -}, []); - -useEffect(() => { - window.addEventListener("revertToPreviousScreenFromFormula", goToPreviousScreenFromFormula); + const events = ["moveToPrevious", "revertToPreviousScreenFromFormula"]; + events.forEach(event => { + window.addEventListener(event, () => handleNavigation(event)); + }); return () => { - window.removeEventListener("revertToPreviousScreenFromFormula", goToPreviousScreenFromFormula); + events.forEach(event => { + window.removeEventListener(event, () => handleNavigation(event)); + }); }; }, []);health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PlanInbox.js (1)
Line range hint
1-549
: Consider architectural improvements for better maintainability and performance.The component has grown complex with multiple responsibilities. Consider these architectural improvements:
State Management:
- Consider using useReducer for complex state
- Extract table-related state into a custom hook
- Memoize expensive computations
Error Handling:
- Add error boundaries
- Show error states for failed API calls
- Add retry mechanisms for failed requests
Performance:
- Memoize callback functions with useCallback
- Memoize derived data with useMemo
- Split into smaller components
Example implementation for table state extraction:
const useTableState = (initialRowsPerPage = 5) => { const [currentPage, setCurrentPage] = useState(1); const [rowsPerPage, setRowsPerPage] = useState(initialRowsPerPage); const [selectedRows, setSelectedRows] = useState([]); const limitAndOffset = useMemo(() => ({ limit: rowsPerPage, offset: (currentPage - 1) * rowsPerPage }), [currentPage, rowsPerPage]); return { currentPage, setCurrentPage, rowsPerPage, setRowsPerPage, selectedRows, setSelectedRows, limitAndOffset }; };Example error boundary implementation:
const withErrorBoundary = (WrappedComponent) => { return class extends React.Component { state = { hasError: false }; static getDerivedStateFromError(error) { return { hasError: true }; } render() { if (this.state.hasError) { return <Card>Error loading plan inbox. Please try again.</Card>; } return <WrappedComponent {...this.props} />; } }; }; export default withErrorBoundary(PlanInbox);
📜 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/CampaignDetails.js (2 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 (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.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
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js
[error] 29-29: 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)
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js (1)
90-137
: 🛠️ Refactor suggestionExtract common dropdown props and implement error handling.
The dropdowns share common props and have commented-out error handling code.
Consider creating a reusable dropdown component:
const CommonDropdown = ({ label, options, optionKey, selected, onSelect, disabled }) => ( <LabelFieldPair> <div className="campaign-type" style={{ minWidth: "17rem" }}> <span>{t(label)}</span> <span className="mandatory-span">*</span> </div> <Dropdown style={{ width: "40rem", paddingBottom: "1rem" }} t={t} option={options} optionKey={optionKey} selected={selected} select={onSelect} disabled={disabled} /> </LabelFieldPair> );Also, please implement proper error handling instead of leaving it commented out.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1)
42-42
:⚠️ Potential issueValidate totalFormData before initialization
The initialization of microplanConfig now depends on totalFormData instead of params, but there's no validation to ensure totalFormData is properly initialized. This could lead to undefined behavior if totalFormData is empty during initial render.
Let's verify the initialization flow:
Consider adding validation:
-const [microplanConfig, setMicroplanConfig] = useState(MicroplanConfig(totalFormData, null, isSubmitting, null, hierarchyData)); +const [microplanConfig, setMicroplanConfig] = useState(() => { + if (!totalFormData || Object.keys(totalFormData).length === 0) { + return MicroplanConfig({}, null, isSubmitting, null, hierarchyData); + } + return MicroplanConfig(totalFormData, null, isSubmitting, null, hierarchyData); +});
...micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js
Show resolved
Hide resolved
...micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js
Show resolved
Hide resolved
...micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js
Show resolved
Hide resolved
...micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.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/pages/employee/PlanInbox.js
Show resolved
Hide resolved
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.
Actionable comments posted: 1
🧹 Outside diff range comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (2)
Line range hint
149-164
: Improve error handling in handleUpdatesThe error handling could be enhanced:
- The error message fallback uses t("FAILED_TO_UPDATE_RESOURCE") directly, but the error object might contain more specific information
- Consider adding error logging for debugging
onError: (error, variables) => { setLoader(false) + // Log error for debugging + console.error('Resource update failed:', error); + + // Extract specific error details if available + const errorMessage = error?.response?.data?.message || + error?.message || + t("FAILED_TO_UPDATE_RESOURCE"); setShowToast({ key: "error", - label: error?.message ? error.message : t("FAILED_TO_UPDATE_RESOURCE"), + label: errorMessage, }); },
Line range hint
313-385
: Clean up commented code and simplify popup implementationThere are two popup implementations, with one completely commented out. This makes the code harder to maintain.
- Remove the commented popup implementation if it's no longer needed
- Consider extracting the popup logic into a separate component for better maintainability
- {/* Default popup */} - {/* {showPopUp && (<PopUp - className={"boundaries-pop-module"} - onClose={()=> setShowPopUp(false)} - type={"default"} - heading={t("ES_CAMPAIGN_UPDATE_TYPE_MODAL_HEADER")} - children={[ - <div> - <CardText style={{ margin: 0 }}>{t("ES_CAMPAIGN_UPDATE_TYPE_MODAL_TEXT") + " "}</CardText> - </div>, - ]} - onOverlayClick={() => { - setShowPopUp(false); - }} - footerChildren={[ - <Button - className={"campaign-type-alert-button"} - type={"button"} - size={"large"} - variation={"secondary"} - label={t("Heloo")} - onClick={() => { - setShowPopUp(false); - }} - />, - <Button - className={"campaign-type-alert-button"} - type={"button"} - size={"large"} - variation={"primary"} - label={t("ES_CAMPAIGN_BOUNDARY_MODAL_SUBMIT")} - onClick={() => { - setShowPopUp(false); - // setCanUpdate(false); - }} - />, - ]} - sortFooterChildren={true} - ></PopUp>)} */}
📜 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/CampaignDetails.js (2 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 (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.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
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js
[error] 29-29: 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)
🔇 Additional comments (4)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js (2)
23-27
: LGTM! Well-implemented state synchronization.The useEffect hook correctly synchronizes the component's state with session data, including proper dependency tracking and fallback handling.
29-29
:⚠️ Potential issueSimplify the boolean expression.
The boolean expression can be simplified by removing the redundant ternary operator.
-const [isFreezed, setIsFreezed] = useState(campaignId && microplanId ? true : false); +const [isFreezed, setIsFreezed] = useState(Boolean(campaignId && microplanId));Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 29-29: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1)
42-42
: Consider tracking hierarchyData changesThe microplanConfig initialization uses hierarchyData but changes to hierarchyData won't trigger a recalculation. This could lead to stale configurations if hierarchyData updates.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PlanInbox.js (1)
478-478
: Loading state implementation could be improved further.While showing a loader when any loading state is true is better than waiting for all states, the past review suggestions for using a skeleton loader and showing partial data are still valid to prevent layout shifts and improve user experience.
Let's verify if the Digit UI components support skeleton loading:
#!/bin/bash # Search for skeleton loader component usage rg -i 'skeleton|cardSkeleton' --type js
...micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js
Show resolved
Hide resolved
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.
Actionable comments posted: 1
📜 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/pages/employee/PlanInbox.js (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PlanInbox.js (1)
Pattern
**/*.js
: check
🔇 Additional comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PlanInbox.js (1)
478-478
: Loading state implementation could be improved.A previous review comment already suggested improvements to the loading state implementation, including using skeleton loading and showing partial data. That suggestion is still valid and applicable.
Choose the appropriate template for your PR:
Summary by CodeRabbit
PlanInbox
component with immediate feedback via aLoader
component.CampaignDetails
component for better synchronization with session data.microplanConfig
in theSetupMicroplan
component for improved data handling.PlanInbox
component for a better user experience during data fetching.