HLM-6172 mapping page correction and api call data corrections #845
Conversation
WalkthroughWalkthroughThis update introduces enhancements across various components of the micro UI, particularly focusing on the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Micro UI
participant Map as Map Component
participant Server
User->>UI: Interacts with map
UI->>Map: Calls addGeojsonToMap
Map->>Map: Binds popup with minWidth and padding
Map->>UI: Returns updated map
UI->>User: Displays map with updated pop-ups
User->>UI: Uploads Geojson file
UI->>Server: Validates Geojson file
Server-->>UI: Returns validation result
UI->>User: Displays validation result with potential errors
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 5
Outside diff range and nitpick comments (15)
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js (1)
Line range hint
145-160: Refactor to improve readability and error handling in thehandleGeoJsonfunction. Consider extracting repeated logic into a separate function.+ function createUploadObjectWithError(templateIdentifier, inputFileType, fileId, filestoreId, extension, active) { + return createUploadObject(templateIdentifier, inputFileType, fileId, filestoreId, extension, active); + } - let uploadObject = createUploadObject(templateIdentifier, inputFileType, fileId, filestoreId, shapefileOrigin ? ".zip" : ".geojson", active); - if (!schema) { - console.error("Schema got undefined while handling geojson at handleGeoJson"); - return [...upload, uploadObject]; - }micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js (7)
Line range hint
1344-1365: Ensure accessibility by adding keyboard event handlers to clickable elements.The popup binding in the
addGeojsonToMapfunction uses a mouse event but lacks corresponding keyboard event handlers. This can hinder accessibility for users who rely on keyboard navigation. Consider addingonKeyUp,onKeyDown, oronKeyPressevent handlers to complement theonClickevents.- layer.bindPopup(popupContent, { - minWidth: "28rem", - padding: "0", - }); + layer.bindPopup(popupContent, { + minWidth: "28rem", + padding: "0", + onKeyUp: handleKeyUp, + onKeyDown: handleKeyDown, + });
Line range hint
38-38: ReplacehasOwnPropertywithObject.hasOwn.Using
Object.prototype.hasOwnProperty.call(obj, key)can lead to potential bugs if the object's prototype chain has been modified. It's safer to useObject.hasOwn(obj, key)which is not affected by the prototype chain.- if (obj.hasOwnProperty(key)) { + if (Object.hasOwn(obj, key)) {
Line range hint
266-266: Simplify the use of ternary operators.The use of ternary operators here might be unnecessary. Consider simplifying the expression for better readability.
- child: !childrenList || childrenList.length === 0 ? true : false, + child: !childrenList || childrenList.length === 0,
Line range hint
524-524: Remove inappropriatetabIndexfrom non-interactive elements.Using
tabIndexon non-interactive elements can confuse users navigating with keyboards. Consider removing it or replacing the element with a more appropriate interactive HTML element.- <div className="icon" tabIndex={0}> + <div className="icon">
Line range hint
562-562: Avoid assignments within expressions.Assignments within expressions can lead to code that is hard to read and potentially error-prone. Consider separating the assignment from the expression.
- let tempFilterSelections = [...filterSelections]; // Clone the array to avoid mutating state directly + let tempFilterSelections = [...filterSelections]; + // further operations on tempFilterSelections
Line range hint
917-917: Use optional chaining to simplify deep property access.Optional chaining can make your code cleaner and prevent runtime errors due to undefined or null values in the chain.
- if (feature && feature.properties && feature.properties.addOn && feature.properties.addOn.child) { + if (feature?.properties?.addOn?.child) {Also applies to: 926-926, 927-927, 956-957, 1058-1058, 1060-1060
Line range hint
1068-1068: Wrap declarations in blocks within switch cases.Variables declared within switch cases without a block are scoped to the entire switch statement, which can lead to unexpected behaviors. Enclose the declarations in a block to limit their scope to the case.
case EXCEL: { + let columnList = Object.values(fileData?.data)?.[0]?.[0]; + let check = true; // further operations } break;Also applies to: 1072-1082
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js (7)
Line range hint
151-165: Remove unnecessary else clause to simplify control flow.- } else { - if (navigationEvent?.name === "next") { - if (currentSectionIndex < sections.length - 1) { - setSelectedSection(sections[currentSectionIndex + 1]); - setCheckDataCompletion("false"); - return; - } - } else if (navigationEvent?.name === "previousStep") { - if (currentSectionIndex > 0) { - setSelectedSection(sections[currentSectionIndex - 1]); - setCheckDataCompletion("false"); - return; - } - } - }
Line range hint
1121-1121: Ensure keyboard accessibility by adding keyboard event handlers to clickable elements.+ onKeyUp={handleKeyUp}Add
onKeyUpor similar keyboard event handlers to ensure that all functionalities accessible by mouse are also accessible via the keyboard.Also applies to: 1144-1144
Line range hint
1223-1223: Use self-closing tags for JSX elements without children.- <div className="link" onClick={setModal}> + <div className="link" onClick={setModal} />
Line range hint
1234-1234: Specify thetypeattribute for button elements to prevent unexpected form submissions.+ type="button"Add
type="button"to each button element to ensure they do not inadvertently submit forms.Also applies to: 1273-1273
Line range hint
1380-1380: ReplacehasOwnPropertywithObject.hasOwnfor safer property checks.- if (jsonObject.hasOwnProperty(key)) { + if (Object.hasOwn(jsonObject, key)) {Also applies to: 1695-1695
Line range hint
1360-1362: Convert function expressions to arrow functions for consistency and to avoid issues withthisbinding.- const handleKeyPress = function(event) { + const handleKeyPress = (event) => {
Line range hint
702-702: Avoid reassigning the catch parameter to prevent confusion and potential bugs.- catch (error) { + catch (errorData) {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
micro-ui/web/micro-ui-internals/packages/css/package.jsonis excluded by!**/*.json
Files selected for processing (6)
- micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss (1 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js (2 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/MicroplanDetails.js (4 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js (4 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/geojsonValidations.js (4 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js (7 hunks)
Additional context used
Path-based instructions (5)
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/geojsonValidations.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/MicroplanDetails.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js (1)
Pattern
**/*.js: check
Learnings (3)
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/MicroplanDetails.js (1)
User: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#698 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/MicroplanPreview.js:1-1 Timestamp: 2024-05-27T04:09:43.769Z Learning: The imports in `MicroplanPreview.js` are from different libraries: `@egovernments/digit-ui-components` and `@egovernments/digit-ui-react-components`.micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js (1)
User: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#698 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js:144-156 Timestamp: 2024-05-26T18:47:54.758Z Learning: In the `updateSessionUtils.js` file, during the data processing phase, errors should be logged, and the file should be ignored without throwing exceptions.micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js (3)
User: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:468-481 Timestamp: 2024-03-13T07:33:45.211Z Learning: The error message in the `checkForErrorInUploadedFile` function within `Upload.js` is being localized and improved for better user experience, as clarified by the user.User: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:0-0 Timestamp: 2024-03-13T05:11:36.400Z Learning: A more detailed message for file parsing errors in the `Upload.js` file is displayed elsewhere in the code, as clarified by the user.User: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#214 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:14-17 Timestamp: 2024-03-15T12:20:50.090Z Learning: The `geojsonValidations` is a utility developed within the DIGIT-Frontend project, not an external dependency.
Biome
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/geojsonValidations.js
[error] 28-32: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 30-32: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 155-155: Unsafe usage of optional chaining. (lint/correctness/noUnsafeOptionalChaining)
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/MicroplanDetails.js
[error] 117-117: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 148-153: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 241-241: Avoid using the index of an array as key property in an element. (lint/suspicious/noArrayIndexKey)
This is the source of the key value.
The order of the items may change, and this also affects performances and component state.
Check the React documentation.
[error] 289-289: Missing key property for this element in iterable. (lint/correctness/useJsxKeyInIterable)
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
[error] 291-291: Missing key property for this element in iterable. (lint/correctness/useJsxKeyInIterable)
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
[error] 292-292: Avoid using the index of an array as key property in an element. (lint/suspicious/noArrayIndexKey)
This is the source of the key value.
The order of the items may change, and this also affects performances and component state.
Check the React documentation.
[error] 295-295: Avoid using the index of an array as key property in an element. (lint/suspicious/noArrayIndexKey)
This is the source of the key value.
The order of the items may change, and this also affects performances and component state.
Check the React documentation.micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js
[error] 70-81: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 83-85: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 406-407: This case is falling through to the next case. (lint/suspicious/noFallthroughSwitchClause)
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js
[error] 38-38: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 266-266: Unnecessary use of boolean literals in conditional expression. (lint/complexity/noUselessTernary)
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
[error] 519-519: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 524-524: The HTML element div is non-interactive. Do not use tabIndex. (lint/a11y/noNoninteractiveTabindex)
Adding non-interactive elements to the keyboard navigation flow can confuse users.
Unsafe fix: Remove the tabIndex attribute.
[error] 562-562: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 572-572: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 573-573: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 711-718: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 826-826: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 836-842: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 917-917: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 926-926: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 927-927: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 956-957: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1057-1057: This var should be declared at the root of the enclosing function. (lint/correctness/noInnerDeclarations)
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.
[error] 1058-1058: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1060-1060: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1068-1068: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 1068-1068: This var should be declared at the root of the enclosing function. (lint/correctness/noInnerDeclarations)
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.
[error] 1072-1082: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js
[error] 151-165: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 220-220: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 283-283: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 553-553: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 695-700: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1121-1121: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 1144-1144: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 1212-1215: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1223-1223: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing. (lint/style/useSelfClosingElements)
Unsafe fix: Use a SelfClosingElement instead
[error] 1360-1362: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 1380-1380: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 1390-1443: Promise executor functions should not be
async. (lint/suspicious/noAsyncPromiseExecutor)
[error] 1457-1459: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1583-1585: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1695-1695: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 1804-1804: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 702-702: Reassigning a catch parameter is confusing. (lint/suspicious/noCatchAssign)
The catch parameter is declared here:
Use a local variable instead.
[error] 1142-1142: Missing key property for this element in iterable. (lint/correctness/useJsxKeyInIterable)
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
[error] 1234-1234: Provide an explicit type prop for the button element. (lint/a11y/useButtonType)
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
[error] 1273-1273: Provide an explicit type prop for the button element. (lint/a11y/useButtonType)
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
Additional comments not posted (6)
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/geojsonValidations.js (1)
44-44: The error message array is a good improvement for consistency in handling multiple errors.micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/MicroplanDetails.js (3)
12-12: Good addition ofLoaderWithGapto handle loading states visually.
41-41: Proper use of React state for managing loader visibility.
219-219: Displaying the loader conditionally based on theloaderstate is a good practice.micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js (1)
Line range hint
206-221: Good implementation of conditional logic based onprocessedData. Ensure that all edge cases are handled.micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss (1)
2476-2483: The new.map-pop-upstyle block is well-defined and aligns with the PR objectives to enhance the mapping page. The use ofcustom-scrollbarmixin ensures consistency in scrollbar appearance across the application. The specified dimensions and overflow handling are appropriate for a popup element. Good job on maintaining a clean and structured CSS.
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js
Show resolved
Hide resolved
...ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/geojsonValidations.js
Show resolved
Hide resolved
...ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/geojsonValidations.js
Show resolved
Hide resolved
...web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/MicroplanDetails.js
Show resolved
Hide resolved
...ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js
Outdated
Show resolved
Hide resolved
…g prerequisites from upload data section
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range and nitpick comments (18)
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/MicroplanPreview.js (9)
Line range hint
129-129: Consider using optional chaining to improve code safety and readability.- if (boundarySelections && Object.values(boundarySelections).every((item) => item.length === 0) && hierarchy) { + if (boundarySelections?.every((item) => item.length === 0) && hierarchy) {
Line range hint
274-274: Declare variables at the root of the enclosing function to enhance clarity and prevent scope-related errors.- var tempBoundarySelection = {}; + let tempBoundarySelection = {};
Line range hint
373-373: Remove the duplicate object key to prevent overwriting values unintentionally.- popupModuleActionBarStyles={{ - display: "flex", - flex: 1, - justifyContent: "flex-start", - padding: 0, - width: "100%", - padding: "1rem", - }} + popupModuleActionBarStyles={{ + display: "flex", + flex: 1, + justifyContent: "flex-start", + padding: "1rem", + width: "100%", + }}
Line range hint
701-703: Theelseclause is unnecessary here as the previous branches break early. Simplifying control flow can improve code readability.- else { - setToast({ - message: t("ERROR_DATA_NOT_SAVED"), - state: "error", - }); - setLoaderActivation(true); - setTimeout(() => { - setLoaderActivation(false); - setToast(undefined); - if (navigationEvent.name === "next") setCheckDataCompletion("false"); - else setCheckDataCompletion("perform-action"); - }, 1000); - return; - }
Line range hint
720-729: ReplaceflatMapwithflatto simplify the code since the callback function is unnecessary.- tempdata = filteredSchemas - ?.map((item) => - Object.entries(item?.schema?.Properties || {}).reduce((acc, [key, value]) => { - if (value?.isRuleConfigureInputs && value?.toShowInMicroplanPreview) { - acc.push(key); - } - return acc; - }, []) - ) - .flatMap((item) => item) - .filter((item) => !!item); + tempdata = filteredSchemas + ?.flatMap((item) => + Object.entries(item?.schema?.Properties || {}).reduce((acc, [key, value]) => { + if (value?.isRuleConfigureInputs && value?.toShowInMicroplanPreview) { + acc.push(key); + } + return acc; + }, []) + ) + .filter((item) => !!item);
Line range hint
798-798: Consider using optional chaining to improve code safety and readability.- if (!boundarySelections && !resources) return; + if (!boundarySelections?.length && !resources) return;
Line range hint
862-862: Theelseclause is unnecessary here as the previous branches break early. Simplifying control flow can improve code readability.- else { - setToast({ - message: t("ERROR_DATA_NOT_SAVED"), - state: "error", - }); - setLoaderActivation(true); - setTimeout(() => { - setLoaderActivation(false); - setToast(undefined); - }, 2000); - }
Line range hint
609-609: ReplaceisNaNwithNumber.isNaNto avoid type coercion and improve type safety.- if (isNaN(parseInt(aggregate))) { + if (Number.isNaN(parseInt(aggregate))) {Also applies to: 942-942, 1123-1123, 1210-1210
Line range hint
1123-1123: ReplaceisFinitewithNumber.isFiniteto avoid type coercion and improve type safety.- if (!isFinite(value) && value !== "") return; + if (!Number.isFinite(value) && value !== "") return;micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js (7)
Line range hint
38-38: ReplacehasOwnPropertywithObject.hasOwnfor better safety and future compatibility.- if (obj.hasOwnProperty(key)) { + if (Object.hasOwn(obj, key)) {
Line range hint
529-529: Ensure accessibility by adding keyboard event handlers to elements with mouse event handlers.+ onKeyUp={() => setShowChoroplethOptions((previous) => !previous)} + onKeyDown={() => setShowChoroplethOptions((previous) => !previous)}Also applies to: 582-582, 583-583, 721-728, 836-836, 846-852
Line range hint
534-534: Remove thetabIndexfrom non-interactive elements to improve accessibility.- tabIndex={0}
Line range hint
572-572: Avoid assignments within expressions to enhance code clarity.- let tempFilterSelections = [...filterSelections]; // Clone the array to avoid mutating state directly + const tempFilterSelections = [...filterSelections]; // Clone the array to avoid mutating state directly
Line range hint
927-927: Use optional chaining to simplify property access and improve code robustness.- let resources = state?.Resources?.find((item) => item.campaignType === campaignType)?.data; + let resources = state?.Resources?.find?.((item) => item.campaignType === campaignType)?.data;Also applies to: 936-936, 937-937, 966-967, 1068-1068, 1070-1070
Line range hint
1067-1067: Declare variables at the root of the function to avoid confusion and potential bugs.+ let keys, values;Also applies to: 1078-1078
Line range hint
1078-1078: Wrap declarations in a block within the switch clause to restrict their scope.+ { let keys = Object.keys(fileData?.data.features[0].properties); keys.push("feature"); + }Also applies to: 1082-1092
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js (2)
Line range hint
1126-1126: Ensure accessibility by adding keyboard event handlers for actions triggered by mouse events.<button className="download-template-button" onClick={downloadTemplateHandler} tabIndex="0" onKeyPress={downloadTemplateHandler}> <button className="delete-button" onClick={DeleteFile} tabIndex="0" onKeyPress={DeleteFile}>Adding
onKeyPresshandlers ensures that the functionality is accessible via keyboard, enhancing accessibility.Also applies to: 1149-1149
Line range hint
1239-1239: Specify thetypeattribute for buttons to prevent unintended form submissions.<button type="button" className="download-template-button" onClick={downloadTemplateHandler} tabIndex="0"> <button type="button" className="button" onClick={ReuplaodFile} tabIndex="0"> <button type="button" className="delete-button" onClick={DeleteFile} tabIndex="0">Explicitly setting the
typeattribute to "button" prevents these elements from submitting forms unintentionally.Also applies to: 1278-1278, 1295-1295
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss (2 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js (7 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/MicroplanPreview.js (4 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js (10 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/createTemplate.js (7 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js (10 hunks)
- micro-ui/web/public/index.html (1 hunks)
Files skipped from review due to trivial changes (1)
- micro-ui/web/public/index.html
Files skipped from review as they are similar to previous changes (1)
- micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss
Additional context used
Path-based instructions (5)
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/createTemplate.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/MicroplanPreview.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js (1)
Pattern
**/*.js: check
Learnings (3)
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js (1)
User: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#698 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js:144-156 Timestamp: 2024-05-26T18:47:54.758Z Learning: In the `updateSessionUtils.js` file, during the data processing phase, errors should be logged, and the file should be ignored without throwing exceptions.micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/MicroplanPreview.js (2)
User: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#606 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/index.js:276-276 Timestamp: 2024-05-21T07:42:28.329Z Learning: The `addResourcesToFilteredDataToShow` function in `micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/index.js` now includes checks for data integrity, such as validation of `previewData`, the common column, and `resources`, along with error logging for invalid data formats.User: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#675 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/MicroplanPreview.js:4-10 Timestamp: 2024-05-23T05:09:21.739Z Learning: Imports in `MicroplanPreview.js` are either already grouped appropriately or come from different directories, as clarified by the user.micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js (3)
User: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:468-481 Timestamp: 2024-03-13T07:33:45.211Z Learning: The error message in the `checkForErrorInUploadedFile` function within `Upload.js` is being localized and improved for better user experience, as clarified by the user.User: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#204 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:0-0 Timestamp: 2024-03-13T05:11:36.400Z Learning: A more detailed message for file parsing errors in the `Upload.js` file is displayed elsewhere in the code, as clarified by the user.User: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#214 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/pages/employee/Upload.js:14-17 Timestamp: 2024-03-15T12:20:50.090Z Learning: The `geojsonValidations` is a utility developed within the DIGIT-Frontend project, not an external dependency.
Biome
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/createTemplate.js
[error] 32-38: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 35-38: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js
[error] 70-81: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 83-85: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 406-407: This case is falling through to the next case. (lint/suspicious/noFallthroughSwitchClause)
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/MicroplanPreview.js
[error] 129-129: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 274-274: This var should be declared at the root of the enclosing function. (lint/correctness/noInnerDeclarations)
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.
[error] 373-373: This property value named padding is later overwritten by an object member with the same name. (lint/suspicious/noDuplicateObjectKeys)
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named padding
[error] 701-703: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 720-729: Avoid unnecessary callback in flatMap call. (lint/correctness/noFlatMapIdentity)
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.
[error] 733-740: Avoid unnecessary callback in flatMap call. (lint/correctness/noFlatMapIdentity)
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.
[error] 798-798: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 862-862: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1044-1046: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1059-1059: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1075-1075: Avoid unnecessary callback in flatMap call. (lint/correctness/noFlatMapIdentity)
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.
[error] 1091-1091: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 1091-1091: This var should be declared at the root of the enclosing function. (lint/correctness/noInnerDeclarations)
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.
[error] 1094-1103: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 1105-1105: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 609-609: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead. (lint/suspicious/noGlobalIsNan)
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
[error] 942-942: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead. (lint/suspicious/noGlobalIsNan)
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
[error] 1123-1123: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead. (lint/suspicious/noGlobalIsNan)
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
[error] 1123-1123: isFinite is unsafe. It attempts a type coercion. Use Number.isFinite instead. (lint/suspicious/noGlobalIsFinite)
See the MDN documentation for more details.
Unsafe fix: Use Number.isFinite instead.
[error] 1210-1210: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead. (lint/suspicious/noGlobalIsNan)
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js
[error] 38-38: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 271-271: Unnecessary use of boolean literals in conditional expression. (lint/complexity/noUselessTernary)
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
[error] 529-529: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 534-534: The HTML element div is non-interactive. Do not use tabIndex. (lint/a11y/noNoninteractiveTabindex)
Adding non-interactive elements to the keyboard navigation flow can confuse users.
Unsafe fix: Remove the tabIndex attribute.
[error] 572-572: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 582-582: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 583-583: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 721-728: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 836-836: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 846-852: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 927-927: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 936-936: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 937-937: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 966-967: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1067-1067: This var should be declared at the root of the enclosing function. (lint/correctness/noInnerDeclarations)
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.
[error] 1068-1068: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1070-1070: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1078-1078: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 1078-1078: This var should be declared at the root of the enclosing function. (lint/correctness/noInnerDeclarations)
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.
[error] 1082-1092: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js
[error] 151-165: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 220-220: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 283-283: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 556-556: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 698-703: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1126-1126: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 1149-1149: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 1217-1220: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1365-1367: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 1385-1385: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 1395-1448: Promise executor functions should not be
async. (lint/suspicious/noAsyncPromiseExecutor)
[error] 1462-1464: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1588-1590: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1701-1701: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 1810-1810: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 705-705: Reassigning a catch parameter is confusing. (lint/suspicious/noCatchAssign)
The catch parameter is declared here:
Use a local variable instead.
[error] 1147-1147: Missing key property for this element in iterable. (lint/correctness/useJsxKeyInIterable)
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
[error] 1239-1239: Provide an explicit type prop for the button element. (lint/a11y/useButtonType)
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
[error] 1278-1278: Provide an explicit type prop for the button element. (lint/a11y/useButtonType)
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
[error] 1295-1295: Provide an explicit type prop for the button element. (lint/a11y/useButtonType)
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
Additional comments not posted (1)
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js (1)
431-431: The conditional check here is crucial for determining whether to stop the upload process based on the response fromhandleGeojsonFile. However, consider simplifying the condition for readability.- if (response.check === false && response.stopUpload) { + if (!response.check && response.stopUpload) {This change uses logical NOT to simplify the condition, making the code cleaner and easier to understand.
Likely invalid or redundant comment.
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/createTemplate.js
Show resolved
Hide resolved
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/createTemplate.js
Show resolved
Hide resolved
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/createTemplate.js
Show resolved
Hide resolved
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/createTemplate.js
Show resolved
Hide resolved
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js
Outdated
Show resolved
Hide resolved
...ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js
Show resolved
Hide resolved
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js
Show resolved
Hide resolved
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js
Show resolved
Hide resolved
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js
Show resolved
Hide resolved
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Upload.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (9)
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js (3)
Line range hint
192-207: ThehandleGeoJsonandhandleGeoJsonSpecificfunctions are crucial for handling GeoJSON file processing. Here are a few observations:
- Error Handling: The error handling could be more robust. Currently, it logs the error but does not propagate or handle it in a way that the calling function can react to.
- Performance: These functions could potentially be optimized for performance, especially if they are handling large GeoJSON files frequently.
- console.error("Schema got undefined while handling geojson at handleGeoJson"); + throw new Error("Schema got undefined while handling geojson at handleGeoJson");Also applies to: 253-268
Line range hint
70-81: Consider converting the function expressions inparseBlobToJSONandparseGeoJSONResponseinto arrow functions for consistency and to reduce complexity.- function parseBlobToJSON(blob, file) { + const parseBlobToJSON = (blob, file) => { - function parseGeoJSONResponse(arrayBuffer, file) { + const parseGeoJSONResponse = (arrayBuffer, file) => {Also applies to: 83-85
Line range hint
323-398: ThefetchFilesfunction and the subsequentsetMicroplanUploadfunction handle file fetching and processing. Consider the following:
- Error Handling: Ensure that errors from the
axios.getcall are properly caught and handled.- Code Clarity: The function is quite long and handles multiple responsibilities. Breaking it down into smaller functions could improve readability and maintainability.
- // Existing long function + // Suggested breakdown into smaller functionsmicro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js (6)
Line range hint
38-38: ReplacehasOwnPropertywithObject.hasOwnto avoid issues with objects that do not havehasOwnPropertyas a direct property.- if (obj.hasOwnProperty(key)) { + if (Object.hasOwn(obj, key)) {
Line range hint
529-529: Ensure that all clickable elements also have corresponding keyboard event handlers to improve accessibility.+ onKeyUp={handleKeyUp}Add
onKeyUphandlers similar to theonClickhandlers for all interactive elements.Also applies to: 582-582, 583-583, 721-728, 836-836, 846-852
Line range hint
534-534: Thedivelement has atabIndexwhich makes it focusable. Ensure this is intentional and consider if the element should be interactive or if another element type would be more appropriate.- <div tabIndex={0}> + <button>Consider changing the element to a button if it should be interactive.
Line range hint
572-572: Avoid assignments within expressions as they can lead to code that is hard to read and maintain.- let x = (y = 2); + let y = 2; + let x = y;Refactor the code to separate the assignment statements.
Line range hint
927-927: Use optional chaining to make the code more robust and prevent runtime errors due to undefined or null values.- if (obj && obj.prop && obj.prop.value) { + if (obj?.prop?.value) {Refactor the code to use optional chaining for safer property access.
Also applies to: 936-936, 937-937, 966-967, 1068-1068, 1070-1070
Line range hint
1067-1067: Declarations inside switch cases should be scoped properly to avoid errors. Wrap these declarations in blocks.case 'example': + { let x = 1; break; + }Wrap the declarations in blocks to limit their scope to the switch case.
Also applies to: 1078-1078, 1082-1092, 1107-1107
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js (7 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js (10 hunks)
Additional context used
Path-based instructions (2)
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js (1)
Pattern
**/*.js: check
Learnings (1)
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js (1)
User: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#698 File: micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js:144-156 Timestamp: 2024-05-26T18:47:54.758Z Learning: In the `updateSessionUtils.js` file, during the data processing phase, errors should be logged, and the file should be ignored without throwing exceptions.
Biome
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/utils/updateSessionUtils.js
[error] 70-81: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 83-85: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js
[error] 38-38: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 529-529: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 534-534: The HTML element div is non-interactive. Do not use tabIndex. (lint/a11y/noNoninteractiveTabindex)
Adding non-interactive elements to the keyboard navigation flow can confuse users.
Unsafe fix: Remove the tabIndex attribute.
[error] 572-572: The assignment should not be in an expression. (lint/suspicious/noAssignInExpressions)
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 582-582: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 583-583: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 721-728: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 836-836: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 846-852: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 927-927: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 936-936: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 937-937: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 966-967: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1067-1067: This var should be declared at the root of the enclosing function. (lint/correctness/noInnerDeclarations)
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.
[error] 1068-1068: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1070-1070: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 1078-1078: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 1078-1078: This var should be declared at the root of the enclosing function. (lint/correctness/noInnerDeclarations)
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.
[error] 1082-1092: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 1107-1107: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
Additional comments not posted (3)
micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js (3)
135-135: Added a new state variableloaderto manage loading states. Ensure that this state is properly managed across all component updates to prevent memory leaks or state inconsistencies.
Line range hint
188-208: The loading state is set before and after a potentially asynchronous operation. This is a good practice to provide feedback to the user during long operations.
446-446: Display of theLoaderWithGapcomponent based on theloaderstate. Ensure that thetextprop passed toLoaderWithGapis appropriate and that the loader's visibility is correctly toggled.
| const fetchBoundaryDataWrapper = async (schemaData) => { | ||
| let boundaryDataAgainstBoundaryCode = {}; | ||
| // if (!schemaData?.doHierarchyCheckInUploadedData) { | ||
| try { | ||
| const rootBoundary = additionalProps.campaignData?.boundaries?.filter((boundary) => boundary.isRoot); // Retrieve session storage data once and store it in a variable | ||
| const sessionData = Digit.SessionStorage.get("microplanHelperData") || {}; | ||
| let boundaryData = sessionData.filteredBoundaries; | ||
| let filteredBoundaries; | ||
| if (!boundaryData) { | ||
| // Only fetch boundary data if not present in session storage | ||
| boundaryData = await fetchBoundaryData( | ||
| await Digit.ULBService.getCurrentTenantId(), | ||
| additionalProps.campaignData?.hierarchyType, | ||
| rootBoundary?.[0]?.code | ||
| ); | ||
| filteredBoundaries = await filterBoundaries(boundaryData, additionalProps.campaignData?.boundaries); | ||
|
|
||
| // Update the session storage with the new filtered boundaries | ||
| Digit.SessionStorage.set("microplanHelperData", { | ||
| ...sessionData, | ||
| filteredBoundaries: filteredBoundaries, | ||
| }); | ||
| } else { | ||
| filteredBoundaries = boundaryData; | ||
| } | ||
| const xlsxData = addBoundaryData([], filteredBoundaries)?.[0]?.data; | ||
| xlsxData.forEach((item, i) => { | ||
| if (i === 0) return; | ||
| let boundaryCodeIndex = xlsxData?.[0]?.indexOf(commonColumn); | ||
| if (boundaryCodeIndex >= item.length) { | ||
| // If boundaryCodeIndex is out of bounds, return the item as is | ||
| boundaryDataAgainstBoundaryCode[item[boundaryCodeIndex]] = item.slice().map(additionalProps.t); | ||
| } else { | ||
| // Otherwise, remove the element at boundaryCodeIndex | ||
| boundaryDataAgainstBoundaryCode[item[boundaryCodeIndex]] = item | ||
| .slice(0, boundaryCodeIndex) | ||
| .concat(item.slice(boundaryCodeIndex + 1)) | ||
| .map(additionalProps.t); | ||
| } | ||
| }); | ||
| } catch (error) { | ||
| console.error(error?.message); | ||
| } | ||
| // } | ||
| return boundaryDataAgainstBoundaryCode; | ||
| }; |
There was a problem hiding this comment.
The fetchBoundaryDataWrapper function has been updated to include session storage handling and boundary data processing. However, there are a few areas that could be improved:
- Error Handling: While there is a catch block, it only logs the error message. It might be beneficial to handle specific types of errors or retry mechanisms depending on the error type.
- Code Clarity: The nested conditions and multiple responsibilities within this function make it a bit hard to follow. Consider refactoring to separate concerns, such as fetching data and processing it into separate functions.
- // Existing code
+ // Suggested refactoring into smaller, more focused functionsCommittable suggestion was skipped due to low confidence.
| try { | ||
| removeAllLayers(map, layers); | ||
| const { filteredSelection, childrenList } = filterBoundarySelection(boundaryData, boundarySelections); | ||
| let newLayer = []; | ||
| let addOn = { | ||
| fillColor: "rgba(255, 107, 43, 0)", | ||
| weight: 3.5, | ||
| opacity: 1, | ||
| color: "rgba(176, 176, 176, 1)", | ||
| fillOpacity: 0, | ||
| fill: "rgb(4,136,219,1)", | ||
| child: !childrenList || childrenList.length === 0, // so that this layer also has mounse in and mouse out events | ||
| }; | ||
| let geojsonsBase = prepareGeojson(boundaryData, "ALL", addOn); | ||
| if (geojsonsBase) { | ||
| let baseLayer = addGeojsonToMap(map, geojsonsBase, t); | ||
| if (baseLayer) newLayer.push(baseLayer); | ||
| let bounds = findBounds(geojsonsBase); | ||
| if (bounds) map.fitBounds(bounds); | ||
| } | ||
|
|
||
| let geojsonLayer; | ||
| if (choroplethProperty) { | ||
| if (dataCompleteness === "partial" || dataCompleteness === "false" || dataCompleteness === undefined) { | ||
| setToast({ | ||
| state: "warning", | ||
| message: t("DISPLAYING_DATA_ONLY_FOR_UPLOADED_BOUNDARIES"), | ||
| }); | ||
| addOn = { | ||
| fillColor: "rgba(255, 107, 43, 1)", | ||
| weight: 2.5, | ||
| opacity: 1, | ||
| color: "rgba(255, 255, 255, 1)", | ||
| fillOpacity: 0.22, | ||
| fill: "rgb(4,136,219)", | ||
| }; | ||
|
|
||
| let geojsonLayer; | ||
| if (choroplethProperty) { | ||
| if (dataCompleteness === "partial" || dataCompleteness === "false" || dataCompleteness === undefined) { | ||
| setToast({ | ||
| state: "warning", | ||
| message: t("DISPLAYING_DATA_ONLY_FOR_UPLOADED_BOUNDARIES"), | ||
| }); | ||
| } | ||
| let choroplethGeojson = prepareGeojson(boundaryData, "ALL", { ...addOn, child: true, fillColor: "rgb(0,0,0,0)" }) || []; | ||
| if (choroplethGeojson && choroplethGeojson.length !== 0) | ||
| choroplethGeojson = addChoroplethProperties(choroplethGeojson, choroplethProperty, filteredSelection); | ||
| geojsonLayer = addGeojsonToMap(map, choroplethGeojson, t); | ||
| if (geojsonLayer) { | ||
| newLayer.push(geojsonLayer); | ||
| } | ||
| } | ||
| let choroplethGeojson = prepareGeojson(boundaryData, "ALL", { ...addOn, child: true, fillColor: "rgb(0,0,0,0)" }) || []; | ||
| if (choroplethGeojson && choroplethGeojson.length !== 0) | ||
| choroplethGeojson = addChoroplethProperties(choroplethGeojson, choroplethProperty, filteredSelection); | ||
| geojsonLayer = addGeojsonToMap(map, choroplethGeojson, t); | ||
| if (geojsonLayer) { | ||
| geojsonLayer = null; | ||
| const geojsons = prepareGeojson(boundaryData, filteredSelection, addOn); | ||
| if (geojsons && geojsons.length > 0) { | ||
| geojsonLayer = addGeojsonToMap(map, geojsons, t); | ||
| newLayer.push(geojsonLayer); | ||
| let bounds = findBounds(geojsons); | ||
| if (bounds) map.fitBounds(bounds); | ||
| } | ||
| } | ||
| geojsonLayer = null; | ||
| const geojsons = prepareGeojson(boundaryData, filteredSelection, addOn); | ||
| if (geojsons && geojsons.length > 0) { | ||
| geojsonLayer = addGeojsonToMap(map, geojsons, t); | ||
| newLayer.push(geojsonLayer); | ||
| let bounds = findBounds(geojsons); | ||
| if (bounds) map.fitBounds(bounds); | ||
| } | ||
|
|
||
| const childrenGeojson = prepareGeojson(boundaryData, childrenList, { ...addOn, opacity: 0, fillOpacity: 0, child: true }); | ||
| let childrenGeojsonLayer = addGeojsonToMap(map, childrenGeojson, t); | ||
| if (childrenGeojsonLayer) newLayer.push(childrenGeojsonLayer); | ||
| const childrenGeojson = prepareGeojson(boundaryData, childrenList, { ...addOn, opacity: 0, fillOpacity: 0, child: true }); | ||
| let childrenGeojsonLayer = addGeojsonToMap(map, childrenGeojson, t); | ||
| if (childrenGeojsonLayer) newLayer.push(childrenGeojsonLayer); | ||
|
|
||
| //filters | ||
| const filterGeojsons = prepareGeojson(filterData, filteredSelection && filteredSelection.length !== 0 ? filteredSelection : "ALL", addOn); | ||
| const filterGeojsonWithProperties = addFilterProperties(filterGeojsons, filterSelections, filterPropertyNames, state?.MapFilters); | ||
| let filterGeojsonLayer = addGeojsonToMap(map, filterGeojsonWithProperties, t); | ||
| if (filterGeojsonLayer) newLayer.push(filterGeojsonLayer); | ||
| //filters | ||
| const filterGeojsons = prepareGeojson(filterData, filteredSelection && filteredSelection.length !== 0 ? filteredSelection : "ALL", addOn); | ||
| const filterGeojsonWithProperties = addFilterProperties(filterGeojsons, filterSelections, filterPropertyNames, state?.MapFilters); | ||
| let filterGeojsonLayer = addGeojsonToMap(map, filterGeojsonWithProperties, t); | ||
| if (filterGeojsonLayer) newLayer.push(filterGeojsonLayer); | ||
|
|
||
| setLayer(newLayer); | ||
| setLayer(newLayer); | ||
| } catch (error) { | ||
| console.error("Error while adding geojson to map: ", error.message); | ||
| } | ||
| setLoader(false); |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that map.removeLayer(layer); is not used within any of the useEffect hooks or lifecycle methods in the Mapping.js file. This suggests that the cleanup logic for the layers might not be properly implemented, which could lead to potential memory leaks.
- Ensure that all new layers added in the provided code snippet are properly cleaned up.
- Implement the cleanup logic within appropriate lifecycle methods or
useEffecthooks to prevent memory leaks.
Analysis chain
Significant changes in the handling of geojson layers. Ensure that all new layers and properties are correctly managed and that there are no memory leaks with the layers added to the map.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify proper cleanup of layers to prevent memory leaks.
# Test: Search for cleanup logic in the component.
rg --type javascript $'removeLayer' -- 'Mapping.js'
Length of output: 83
Script:
#!/bin/bash
# Description: Verify proper cleanup of layers to prevent memory leaks.
# Test: Search for cleanup logic in the component.
rg 'removeLayer' Mapping.js
Length of output: 78
Script:
#!/bin/bash
# Description: Locate the Mapping.js file and verify proper cleanup of layers to prevent memory leaks.
# Step 1: Locate the Mapping.js file.
fd Mapping.js
# Step 2: Once located, search for cleanup logic in the component.
fd Mapping.js --exec rg 'removeLayer' {}
Length of output: 280
Script:
#!/bin/bash
# Description: Verify the context and usage of map.removeLayer(layer); in Mapping.js to ensure proper cleanup of layers.
# Step 1: Search for the usage of map.removeLayer(layer); in Mapping.js.
rg 'map.removeLayer(layer);' micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js
# Step 2: Search for the functions where map.removeLayer(layer); is used to understand the context.
rg 'function ' micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js
Length of output: 979
Script:
#!/bin/bash
# Description: Verify the context and usage of map.removeLayer(layer); in Mapping.js to ensure proper cleanup of layers.
# Step 1: Search for lifecycle methods or event handlers in the Mapping.js file.
rg 'componentDidMount|componentWillUnmount|useEffect|addEventListener|removeEventListener' micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js
# Step 2: Verify if map.removeLayer(layer); is used in these contexts.
rg 'map.removeLayer(layer);' micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js
Length of output: 551
Script:
#!/bin/bash
# Description: Verify the context and usage of map.removeLayer(layer); in Mapping.js to ensure proper cleanup of layers.
# Step 1: Search for the usage of map.removeLayer(layer); within the useEffect hooks.
rg 'useEffect' -A 10 micro-ui/web/micro-ui-internals/packages/modules/hcm-microplanning/src/components/Mapping.js | rg 'map.removeLayer(layer);'
Length of output: 144
No description provided.