-
Notifications
You must be signed in to change notification settings - Fork 19
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
patch fix #1868
patch fix #1868
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several updates across three 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 (
|
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 (7)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/ChooseActivity.js (3)
Line range hint
51-63
: Optimize the process instance API hook configuration.Consider the following improvements:
- Add null check for
microplanId
to prevent unnecessary API calls- Cache the response using proper cache key
const { isLoading:isProcessLoading, data: processData, revalidate, refetch:refetchProcessInstance } = Digit.Hooks.useCustomAPIHook({ url: "/egov-workflow-v2/egov-wf/process/_search", params: { tenantId: tenantId, history: true, businessIds: microplanId, }, config: { - enabled: true, + enabled: Boolean(microplanId), + cacheTime: 5000, + staleTime: 3000, select: (data) => { return data?.ProcessInstances; }, }, });
Line range hint
147-156
: Enhance error handling in onClickCard.The current error handling only logs errors to console. Consider:
- Adding user feedback for failures
- Implementing proper error recovery
- Using a centralized error handling mechanism
const onClickCard = async (card) => { try { if(card.optionKey==="VALIDATE_N_APPROVE_POPULATION_DATA" && planObject.status==="EXECUTION_TO_BE_DONE"){ await updatePlan({ ...planObject, workflow:{ action:"START_DATA_APPROVAL" } }); } } catch (error) { - console.error("ERROR_OCCURED_WHILE_UPDATING_PLAN"); - console.error(error); + Digit.Utils.notificationHandler({ + message: t("ERROR_UPDATING_PLAN"), + type: "error" + }); + // Optionally revert any UI state changes } }
Line range hint
94-117
: Clean up commented code.There's a commented out section for GEOSPATIAL_MAP_VIEW. If this feature is no longer needed, consider removing it completely. If it's for future use, add a TODO comment explaining when it will be implemented.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PlanInbox.js (4)
Line range hint
32-52
: Consider using useReducer for complex state managementThe component uses multiple useState hooks for related state (selectedRows, villagesSelected, currentPage, etc.). Consider using useReducer to manage related state together, making the code more maintainable and easier to debug.
-const [selectedRows, setSelectedRows] = useState([]); -const [villagesSlected, setVillagesSelected] = useState(0); -const [currentPage, setCurrentPage] = useState(1); -const [rowsPerPage, setRowsPerPage] = useState(50); +const [tableState, dispatch] = useReducer(tableReducer, { + selectedRows: [], + villagesSelected: 0, + currentPage: 1, + rowsPerPage: 50 +});🧰 Tools
🪛 Biome
[error] 854-856: 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
571-579
: Enhance error handling for API failuresThe error handling for workflow actions could be improved. Consider adding proper error boundaries and user-friendly error messages.
onError={(data) => { closePopUp(); - setShowToast({ key: "error", label: t(error?.response?.data?.Errors?.[0]?.code) }); + const errorMessage = error?.response?.data?.Errors?.[0]?.code + ? t(error.response.data.Errors[0].code) + : t('COMMON_ERROR_UNKNOWN'); + setShowToast({ + key: "error", + label: errorMessage, + transitionTime: 5000 + }); + console.error('Workflow action failed:', error); }🧰 Tools
🪛 Biome
[error] 854-856: 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
447-538
: Optimize table column definitions using useMemoThe column definitions are recalculated on every render. Consider memoizing them to improve performance.
-const columns = [ +const columns = useMemo(() => [ // ... column definitions -]; +], [t, employeeNameMap, planWithCensus?.planData]);🧰 Tools
🪛 Biome
[error] 854-856: 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-855
: Consider splitting the component into smaller, focused componentsThe file is quite large and handles multiple responsibilities. Consider:
- Extracting the data table into a separate component
- Moving helper functions to utility files
- Creating separate components for filter and action sections
This will improve maintainability and testability.
🧰 Tools
🪛 Biome
[error] 854-856: 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 (3)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/ChooseActivity.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/PopInbox.js
(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/ChooseActivity.js (1)
Pattern **/*.js
: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PlanInbox.js (1)
Pattern **/*.js
: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PopInbox.js (1)
Pattern **/*.js
: check
📓 Learnings (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PopInbox.js (2)
Learnt from: rachana-egov
PR: egovernments/DIGIT-Frontend#1837
File: health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PopInbox.js:559-561
Timestamp: 2024-11-15T10:15:57.370Z
Learning: In `PopInbox.js`, error handling for the `onError` callback in `WorkflowCommentPopUp` is handled within the component, so explicit error handling in the parent component is unnecessary.
Learnt from: rachana-egov
PR: egovernments/DIGIT-Frontend#1832
File: health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PopInbox.js:592-594
Timestamp: 2024-11-14T12:48:23.024Z
Learning: In `PopInbox.js`, when integrating the `ConfirmationPopUp` component, error handling for API failures is managed internally within the component, so explicitly passing an `onError` prop is not necessary.
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/PopInbox.js
[error] 563-569: 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] 601-601: 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] 727-729: 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)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements