Gepspatial map view #1842
Gepspatial map view #1842
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve extensive updates to various components and configurations within the microplan module. Key modifications include enhancements to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MapViewComponent
participant BoundaryFilter
participant BaseMapSwitcher
participant ChoroplethSelection
User->>MapViewComponent: Interacts with map
MapViewComponent->>BoundaryFilter: Requests boundary options
BoundaryFilter-->>MapViewComponent: Returns selected boundaries
MapViewComponent->>BaseMapSwitcher: Requests base map options
BaseMapSwitcher-->>MapViewComponent: Returns selected base map
MapViewComponent->>ChoroplethSelection: Requests choropleth properties
ChoroplethSelection-->>MapViewComponent: Returns selected properties
MapViewComponent->>User: Updates map view
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (78)
health/micro-ui/web/micro-ui-internals/packages/css/src/index.scss (2)
20-20: 🧹 Nitpick (assertive)
Add newline at end of file
Add a newline at the end of the file to follow standard file formatting practices.
19-20: 🧹 Nitpick (assertive)
Consider organizing imports alphabetically
The addition of the mapview import is correct, but consider organizing the employee page imports alphabetically for better maintainability:
@import "./pages/employee/campaign.scss"; @import "./pages/employee/checklist.scss"; @import "./pages/employee/formulaConfig.scss"; +@import "./pages/employee/mapview.scss"; @import "./pages/employee/microplanInbox.scss"; -@import "./pages/employee/checklist.scss"; -@import "./pages/employee/mapview.scss";Committable suggestion skipped: line range outside the PR's diff.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/constants.js (7)
30-30:
⚠️ Potential issueSecurity concern: Avoid hardcoding sensitive information
The
SHEET_PASSWORDshould not be hardcoded in the constants file. Consider moving it to environment variables or a secure configuration management system.
9-9: 🧹 Nitpick (assertive)
Maintain consistent naming convention for constants
The
commonColumnconstant uses camelCase while other constants use UPPER_SNAKE_CASE. Consider renaming it toCOMMON_COLUMNorBOUNDARY_CODE_COLUMNto better reflect its purpose and maintain consistency.-export const commonColumn = "boundaryCode"; +export const BOUNDARY_CODE_COLUMN = "boundaryCode";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const BOUNDARY_CODE_COLUMN = "boundaryCode";
36-36: 🛠️ Refactor suggestion
Use number type for row count
UNPROTECT_TILL_ROWis defined as a string but represents a numeric value. Consider changing it to a number type.-export const UNPROTECT_TILL_ROW = "10000"; +export const UNPROTECT_TILL_ROW = 10000;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const UNPROTECT_TILL_ROW = 10000;
1-8: 🧹 Nitpick (assertive)
Consider grouping data type constants into an enum-like object
The individual data type constants could be consolidated into a single object for better maintainability and type safety.
Consider refactoring like this:
-export const LOCALITY = "Locality"; -export const EXCEL = "Excel"; -export const GEOJSON = "GeoJSON"; -export const SHAPEFILE = "Shapefile"; +export const DataTypes = { + LOCALITY: "Locality", + EXCEL: "Excel", + GEOJSON: "GeoJSON", + SHAPEFILE: "Shapefile", +} as const;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const DataTypes = { LOCALITY: "Locality", EXCEL: "Excel", GEOJSON: "GeoJSON", SHAPEFILE: "Shapefile", } as const;
11-15: 🛠️ Refactor suggestion
Ensure consistency between ACCEPT_HEADERS keys and data type constants
The keys in ACCEPT_HEADERS should reference the data type constants to prevent inconsistencies and make maintenance easier.
export const ACCEPT_HEADERS = { - GeoJSON: "application/geo+json", - Shapefile: "application/shapefile", - Excel: "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + [DataTypes.GEOJSON]: "application/geo+json", + [DataTypes.SHAPEFILE]: "application/shapefile", + [DataTypes.EXCEL]: "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", };Committable suggestion skipped: line range outside the PR's diff.
34-38: 🧹 Nitpick (assertive)
Add JSDoc comments to clarify constant purposes
The purpose of constants like
SCHEMA_PROPERTIES_PREFIXandUPLOADED_DATA_ACTIVE_STATUSisn't immediately clear. Consider adding JSDoc comments to explain their usage.+/** Prefix used for display-related schema properties in the microplan module */ export const SCHEMA_PROPERTIES_PREFIX = "DISPLAY"; +/** Maximum number of rows to leave unprotected in Excel sheets */ export const UNPROTECT_TILL_ROW = "10000"; +/** Status indicator for active uploaded data in the microplan system */ export const UPLOADED_DATA_ACTIVE_STATUS = "MICROPLAN_ACTIVE";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** Prefix used for display-related schema properties in the microplan module */ export const SCHEMA_PROPERTIES_PREFIX = "DISPLAY"; /** Maximum number of rows to leave unprotected in Excel sheets */ export const UNPROTECT_TILL_ROW = "10000"; /** Status indicator for active uploaded data in the microplan system */ export const UPLOADED_DATA_ACTIVE_STATUS = "MICROPLAN_ACTIVE";
17-23: 💡 Codebase verification
Based on the search results, I can now provide a comprehensive response about the choropleth map gradient and color accessibility:
Consider using a more accessible color scheme and add intermediate gradient points
- The current gradient only uses two points (0% -> #edd1cf, 100% -> #b52626) which may not provide enough visual distinction for data visualization.
- The red color scheme (#b52626) may present accessibility issues for colorblind users. Consider using:
- A more colorblind-friendly sequential color scheme (e.g., blue to purple)
- At least 3-4 intermediate points (e.g., at 25%, 50%, 75%) for smoother transitions
- The primary theme color (#C84C0E) should be validated for sufficient contrast with background colors.
The codebase shows no evidence of color contrast or WCAG accessibility testing setup, making these changes important for improving accessibility.
🔗 Analysis chain
Verify color accessibility and consider adding intermediate gradient points
A few suggestions for the map visualization constants:
- The gradient only has start and end points. Consider adding intermediate points for better visualization.
- Verify that the chosen colors meet WCAG accessibility guidelines.
Let me search for any existing color-related utilities or theme configurations that might provide more context about color usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any color contrast or accessibility testing setup in the codebase rg -i "wcag|contrast|accessibility" --type jsLength of output: 9790
Script:
#!/bin/bash # Search for color-related utilities, theme configurations, or style guides rg -i "theme|color.*constants|style.*guide" --type js -A 3 # Also check if there are any existing intermediate gradient points in other choropleth maps rg -i "choropleth|gradient" --type js -A 3Length of output: 106237
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MapFilterIndex.js (4)
1-41: 💡 Codebase verification
Based on the gathered information, I can now provide the final response:
CSS classes need to be defined for map filter components
The component integration with the parent MapViewComponent is correct and the translation function
tis properly used. However, the required CSS classes (filter-index,filter-row,map-filter-layers) are not found in any CSS files in the project.
- Add the missing CSS classes either in the MapViewComponent's CSS file or in a new dedicated stylesheet
- Ensure the stylesheet is properly imported where these classes are used
🔗 Analysis chain
Verify integration with map view system
Please ensure that:
- The translation function
tis properly connected to the i18n system- The component correctly integrates with the parent map view component
- The styling classes (
filter-index,filter-row,map-filter-layers) are defined in the corresponding CSS/SCSS files
Let me gather more information about the integration and styling.
Let me check one more aspect about the styling to make a complete assessment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for style definitions and component integration echo "Checking for style definitions..." rg -l "filter-index|filter-row|map-filter-layers" --type css --type scss echo "Checking for component usage..." rg -l "MapFilterIndex" --type jsLength of output: 782
Script:
#!/bin/bash # Check the parent component integration echo "Checking MapViewComponent integration..." rg -A 10 "MapFilterIndex" health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MapViewComponent.js echo "Checking for style definitions in CSS files..." fd -e css . health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src --exec rg -l "filter-index|filter-row|map-filter-layers" {} echo "Checking translation usage..." ast-grep --pattern 't($$$)' health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MapFilterIndex.jsLength of output: 1441
Script:
#!/bin/bash # Check for global styles and module styles echo "Checking for style files in the project..." fd -e css -e scss . health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src echo "Checking MapViewComponent for style imports..." rg -l "import.*\.s?css" health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MapViewComponent.js echo "Checking Module.js for style imports..." rg -l "import.*\.s?css" health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Module.jsLength of output: 712
🧰 Tools
🪛 Biome
[error] 7-7: This let declares a variable that is only assigned once.
'temp' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 8-8: This let declares a variable that is only assigned once.
'DynamicIcon' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
1-4: 🧹 Nitpick (assertive)
Optimize imports and icon collection
- Remove unused
Fragmentimport- Consider importing only the required SVG components instead of importing everything with
*-import React,{Fragment} from "react"; +import React from "react"; -import * as DigitSvgs from "@egovernments/digit-ui-svg-components"; +import { /* specific icons */ } from "@egovernments/digit-ui-svg-components"; -const IconCollection = {...DigitSvgs }; +const IconCollection = { /* specific icons */ };Committable suggestion skipped: line range outside the PR's diff.
6-20: 🛠️ Refactor suggestion
Clean up code and improve type safety
- Use
constfor variables that aren't reassigned- Remove commented code
- Consider adding PropTypes or TypeScript for better type safety
+import PropTypes from 'prop-types'; export const FilterItemBuilder = ({ item, MapFilters, t }) => { - let temp = MapFilters?.find((e) => e?.name === item)?.icon?.index; - let DynamicIcon = IconCollection?.[temp]; - // let icon; - // if (typeof DynamicIcon === "function") icon = DynamicIcon({}); + const temp = MapFilters?.find((e) => e?.name === item)?.icon?.index; + const DynamicIcon = IconCollection?.[temp]; return DynamicIcon && typeof DynamicIcon === "function" ? ( <div className="filter-row"> <DynamicIcon width={"1.5rem"} height={"1.5rem"} fill={"white"} /> <p className="map-filter-layers">{t(item)}</p> </div> ) : ( - // <div style={{width:"1.5rem"}}></div> "" ); }; +FilterItemBuilder.propTypes = { + item: PropTypes.string.isRequired, + MapFilters: PropTypes.arrayOf( + PropTypes.shape({ + name: PropTypes.string, + icon: PropTypes.shape({ + index: PropTypes.string + }) + }) + ).isRequired, + t: PropTypes.func.isRequired +};Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 7-7: This let declares a variable that is only assigned once.
'temp' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 8-8: This let declares a variable that is only assigned once.
'DynamicIcon' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
22-39: 🛠️ Refactor suggestion
Improve component implementation
- Remove commented JSX
- Use
nullinstead of empty string for better React practices- Add prop validation
const MapFilterIndex = ({ filterSelections, MapFilters, t }) => { return ( <div className="filter-index"> {filterSelections && filterSelections.length > 0 ? ( <> {filterSelections.map((item, index) => ( - // <div className="filter-row"> <FilterItemBuilder key={item?.id || index} item={item} MapFilters={MapFilters} t={t} /> - // <p>{t(item)}</p> - // </div> ))} </> ) : ( - "" + null )} </div> ); }; +MapFilterIndex.propTypes = { + filterSelections: PropTypes.arrayOf(PropTypes.string).isRequired, + MapFilters: PropTypes.arrayOf( + PropTypes.shape({ + name: PropTypes.string, + icon: PropTypes.shape({ + index: PropTypes.string + }) + }) + ).isRequired, + t: PropTypes.func.isRequired +};Committable suggestion skipped: line range outside the PR's diff.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CustomScaleControl.js (4)
1-5: 🧹 Nitpick (assertive)
Add PropTypes validation for the map prop.
While the early return pattern is good, adding PropTypes would improve type safety and documentation.
Add the following:
+import PropTypes from 'prop-types'; import React, { useEffect, useState } from "react"; const CustomScaleControl = ({ map }) => { if (!map) return null; const [scaleText, setScaleText] = useState(""); + +CustomScaleControl.propTypes = { + map: PropTypes.object.isRequired +};Committable suggestion skipped: line range outside the PR's diff.
19-31: 🧹 Nitpick (assertive)
Optimize performance with useCallback.
The
updateScalefunction is recreated on every render. Memoize it with useCallback to optimize performance.Apply this change:
+import React, { useEffect, useState, useCallback } from "react"; -const updateScale = () => { +const updateScale = useCallback(() => { // ... existing implementation -}; +}, [map]);Committable suggestion skipped: line range outside the PR's diff.
33-38: 🧹 Nitpick (assertive)
Enhance accessibility for screen readers.
While ARIA attributes are used, the scale value could be more descriptive for screen readers.
Consider this improvement:
- <div className="custom-scale" aria-live="polite"> - {scaleText} + <div className="custom-scale" aria-live="polite" role="status"> + {`Map scale: ${scaleText}`} <div className="border-spikes" aria-hidden="true" /> </div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.return ( <div className="custom-scale" aria-live="polite" role="status"> {`Map scale: ${scaleText}`} <div className="border-spikes" aria-hidden="true" /> </div> );
7-17: 🛠️ Refactor suggestion
Improve scale calculation precision and maintainability.
The scale calculation logic could be enhanced for better maintainability and precision.
Consider these improvements:
+// Constants for scale calculation +const SCALE_PIXEL_WIDTH = 100; // Width in pixels used for distance calculation +const METERS_TO_KM = 1000; +const ROUND_TO_NEAREST = 10; + const updateScale = () => { - const maxWidthMeters = map.containerPointToLatLng([0, map.getSize().y]).distanceTo(map.containerPointToLatLng([100, map.getSize().y])); - const scale = maxWidthMeters / 1000; // Convert to kilometers + const maxWidthMeters = map.containerPointToLatLng([0, map.getSize().y]) + .distanceTo(map.containerPointToLatLng([SCALE_PIXEL_WIDTH, map.getSize().y])); + const scale = maxWidthMeters / METERS_TO_KM; // Format the scale text - const scaleTextData = scale < 1 ? `${Math.round(scale * 1000)} m` : `${Math.round(Math.round(scale.toFixed(0) / 10) * 10)} km`; + const scaleTextData = scale < 1 + ? `${Math.round(scale * METERS_TO_KM)} m` + : `${Math.round(Math.round(scale.toFixed(1) / ROUND_TO_NEAREST) * ROUND_TO_NEAREST)} km`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Constants for scale calculation const SCALE_PIXEL_WIDTH = 100; // Width in pixels used for distance calculation const METERS_TO_KM = 1000; const ROUND_TO_NEAREST = 10; const updateScale = () => { // Calculate the scale based on the map's current zoom level const maxWidthMeters = map.containerPointToLatLng([0, map.getSize().y]) .distanceTo(map.containerPointToLatLng([SCALE_PIXEL_WIDTH, map.getSize().y])); const scale = maxWidthMeters / METERS_TO_KM; // Format the scale text const scaleTextData = scale < 1 ? `${Math.round(scale * METERS_TO_KM)} m` : `${Math.round(Math.round(scale.toFixed(1) / ROUND_TO_NEAREST) * ROUND_TO_NEAREST)} km`; // Update the scale text in the container element setScaleText(scaleTextData); };health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/BaseMapSwitcher.js (6)
30-30: 🧹 Nitpick (assertive)
Move inline styles to CSS classes
Inline styles should be moved to CSS classes for better maintainability.
- style={{display:"flex"}} + className="base-map-selector__toggle" - <div className="base-map-area" style={{display:"flex"}}> + <div className="base-map-area base-map-area--flex">Add these classes to your CSS file:
.base-map-selector__toggle { display: flex; } .base-map-area--flex { display: flex; }Also applies to: 37-37
1-5: 🧹 Nitpick (assertive)
Clean up empty lines between imports
Remove the extra empty line after imports to maintain consistent spacing.
import "leaflet/dist/leaflet.css"; import React from "react"; import * as DigitSvgs from "@egovernments/digit-ui-svg-components"; -📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import "leaflet/dist/leaflet.css"; import React from "react"; import * as DigitSvgs from "@egovernments/digit-ui-svg-components";
21-22: 🧹 Nitpick (assertive)
Add PropTypes validation
Add PropTypes validation to ensure proper type checking of component props.
+import PropTypes from 'prop-types'; const BaseMapSwitcher = ({ baseMaps, showBaseMapSelector, setShowBaseMapSelector, handleBaseMapToggle, selectedBaseMapName, basemapRef, t }) => { if (!baseMaps) return null; +BaseMapSwitcher.propTypes = { + baseMaps: PropTypes.object.isRequired, + showBaseMapSelector: PropTypes.bool.isRequired, + setShowBaseMapSelector: PropTypes.func.isRequired, + handleBaseMapToggle: PropTypes.func.isRequired, + selectedBaseMapName: PropTypes.string, + basemapRef: PropTypes.object, + t: PropTypes.func.isRequired +};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import PropTypes from 'prop-types'; const BaseMapSwitcher = ({ baseMaps, showBaseMapSelector, setShowBaseMapSelector, handleBaseMapToggle, selectedBaseMapName, basemapRef, t }) => { if (!baseMaps) return null; BaseMapSwitcher.propTypes = { baseMaps: PropTypes.object.isRequired, showBaseMapSelector: PropTypes.bool.isRequired, setShowBaseMapSelector: PropTypes.func.isRequired, handleBaseMapToggle: PropTypes.func.isRequired, selectedBaseMapName: PropTypes.string, basemapRef: PropTypes.object, t: PropTypes.func.isRequired };
6-17:
⚠️ Potential issueFix critical issues in URL generation logic
There are several issues in this function:
- The
Math.pow(0, zoom)calculation is incorrect - it will always return 1 or undefined- The function lacks input validation for
baseMapUrl- The hardcoded fallback URL needs to be implemented properly
Apply this fix:
export const generatePreviewUrl = (baseMapUrl, center = [0, 0], zoom = 5) => { + if (!baseMapUrl) { + console.warn('Base map URL is required'); + return null; + } + - const lon = Math.floor(((center[1] + 180) / 360) * Math.pow(0, zoom)); + const lon = Math.floor(((center[1] + 180) / 360) * Math.pow(2, zoom)); const lat = Math.floor( ((1 - Math.log(Math.tan((center[0] * Math.PI) / 180) + 1 / Math.cos((center[0] * Math.PI) / 180)) / Math.PI) / 2) * Math.pow(2, zoom) ); - if (baseMapUrl) { - const updatedUrl = baseMapUrl.replace("{z}", zoom).replace("{x}", lat).replace("{y}", lon).replace("{s}", "a"); - return updatedUrl; - } - // Return a default preview URL or handle this case as needed - return "default-preview-url.jpg"; // todo + return baseMapUrl + .replace("{z}", zoom) + .replace("{x}", lat) + .replace("{y}", lon) + .replace("{s}", "a"); };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const generatePreviewUrl = (baseMapUrl, center = [0, 0], zoom = 5) => { if (!baseMapUrl) { console.warn('Base map URL is required'); return null; } const lon = Math.floor(((center[1] + 180) / 360) * Math.pow(2, zoom)); const lat = Math.floor( ((1 - Math.log(Math.tan((center[0] * Math.PI) / 180) + 1 / Math.cos((center[0] * Math.PI) / 180)) / Math.PI) / 2) * Math.pow(2, zoom) ); return baseMapUrl .replace("{z}", zoom) .replace("{x}", lat) .replace("{y}", lon) .replace("{s}", "a"); };
25-34:
⚠️ Potential issueFix accessibility issues in the toggle button
The div is being used as a button but lacks proper accessibility attributes.
- <div - className="icon-first" + <button + className="icon-first base-map-toggle" onClick={() => setShowBaseMapSelector((previous) => !previous)} - onKeyUp={() => setShowBaseMapSelector((previous) => !previous)} - tabIndex={0} - style={{display:"flex"}} + aria-expanded={showBaseMapSelector} + aria-label={t("TOGGLE_BASE_MAP_SELECTOR")} > <p className="map-filter-layers">{t("LAYERS")}</p> <div className="layer-icon">{DigitSvgs.Layers && <DigitSvgs.Layers width={"1.667rem"} height={"1.667rem"} fill={"rgba(255, 255, 255, 1)"} />}</div> - </div> + </button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<button className="icon-first base-map-toggle" onClick={() => setShowBaseMapSelector((previous) => !previous)} aria-expanded={showBaseMapSelector} aria-label={t("TOGGLE_BASE_MAP_SELECTOR")} > <p className="map-filter-layers">{t("LAYERS")}</p> <div className="layer-icon">{DigitSvgs.Layers && <DigitSvgs.Layers width={"1.667rem"} height={"1.667rem"} fill={"rgba(255, 255, 255, 1)"} />}</div> </button>🧰 Tools
🪛 Biome
[error] 29-29: The HTML element div is non-interactive. Do not use tabIndex.
Adding non-interactive elements to the keyboard navigation flow can confuse users.
Unsafe fix: Remove the tabIndex attribute.(lint/a11y/noNoninteractiveTabindex)
40-48:
⚠️ Potential issueImprove accessibility and performance of base map selection
The image click handler needs keyboard support and performance optimization.
- <div key={index} className={`base-map-entity ${name === selectedBaseMapName ? "selected" : ""}`}> + <button + key={name} + className={`base-map-entity ${name === selectedBaseMapName ? "selected" : ""}`} + onClick={() => handleBaseMapToggle(name)} + onKeyPress={(e) => e.key === 'Enter' && handleBaseMapToggle(name)} + aria-label={t(`SELECT_BASE_MAP_${name.toUpperCase()}`)} + aria-pressed={name === selectedBaseMapName} + > <img className="base-map-img" - key={index} src={generatePreviewUrl(baseMap?.metadata?.url, [0, 0], 0)} alt={t("ERROR_LOADING_BASE_MAP")} - onClick={() => handleBaseMapToggle(name)} /> <p>{t(name)}</p> - </div> + </button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<button key={name} className={`base-map-entity ${name === selectedBaseMapName ? "selected" : ""}`} onClick={() => handleBaseMapToggle(name)} onKeyPress={(e) => e.key === 'Enter' && handleBaseMapToggle(name)} aria-label={t(`SELECT_BASE_MAP_${name.toUpperCase()}`)} aria-pressed={name === selectedBaseMapName} > <img className="base-map-img" src={generatePreviewUrl(baseMap?.metadata?.url, [0, 0], 0)} alt={t("ERROR_LOADING_BASE_MAP")} /> <p>{t(name)}</p> </button>🧰 Tools
🪛 Biome
[error] 41-47: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/ChoroplethSelection.js (4)
1-16: 🧹 Nitpick (assertive)
Add prop-types and optimize memoization.
While the component is memoized, it could benefit from additional optimizations and type safety:
- Add PropTypes or TypeScript definitions for better type safety and documentation
- Consider adding a dependency comparison function as the second argument to
memoExample implementation:
import PropTypes from 'prop-types'; const ChoroplethSelection = memo( ({ /* props */ }) => { // component logic }, (prevProps, nextProps) => { // Return true if props are equal (no re-render needed) return ( prevProps.choroplethProperty === nextProps.choroplethProperty && prevProps.showChoroplethOptions === nextProps.showChoroplethOptions ); } ); ChoroplethSelection.propTypes = { choroplethProperties: PropTypes.array.isRequired, showChoroplethOptions: PropTypes.bool.isRequired, showChoroplethOptionRef: PropTypes.object.isRequired, setShowChoroplethOptions: PropTypes.func.isRequired, choroplethProperty: PropTypes.string, setChoroplethProperty: PropTypes.func.isRequired, t: PropTypes.func.isRequired, };
38-70: 🧹 Nitpick (assertive)
Optimize styles and performance.
The current implementation has several areas for improvement:
- Extensive inline styles should be moved to CSS classes
- Radio options mapping could be memoized to prevent unnecessary recalculations
Consider these improvements:
// Move to CSS file .clear-filter-button { width: 100%; display: flex; align-items: center; justify-content: flex-start; border: 0; padding: 0 0.7rem; height: 2.5rem; max-height: 2.5rem; background-color: rgba(250, 250, 250, 1); } // In component const radioOptions = useMemo( () => choroplethProperties.map((item) => ({ name: item, id: item, code: item })), [choroplethProperties] ); // In JSX <Button variation="secondary" className="clear-filter-button" icon={"AutoRenew"} label={t("CLEAR_FILTER")} onClick={() => setChoroplethProperty()} />
17-22: 🛠️ Refactor suggestion
Fix useCallback dependencies.
The
handleChangecallback's dependency array includeschoroplethPropertieswhich isn't used within the callback. This could cause unnecessary recreations of the function.Apply this fix:
const handleChange = useCallback( (value) => { setChoroplethProperty(value?.code); }, - [choroplethProperties] + [setChoroplethProperty] );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const handleChange = useCallback( (value) => { setChoroplethProperty(value?.code); }, [setChoroplethProperty] );
24-37:
⚠️ Potential issueImprove accessibility by using semantic HTML.
The current implementation uses a div with click handlers and tabIndex, which creates accessibility issues. Replace it with a semantic button element.
Apply these changes:
- <div - className="icon-rest virtualization-icon" - onClick={() => setShowChoroplethOptions((previous) => !previous)} - onKeyUp={() => setShowChoroplethOptions((previous) => !previous)} - tabIndex={0} - style={{display:"flex"}} - > + <button + type="button" + className="icon-rest virtualization-icon" + onClick={() => setShowChoroplethOptions((previous) => !previous)} + style={{display:"flex", background: "none", border: "none", width: "100%"}} + > <p>{t("VISUALIZATIONS")}</p> <div className="icon"> {DigitSvgs.FilterAlt && <DigitSvgs.FilterAlt width={"1.667rem"} height={"1.667rem"} fill={"rgba(255, 255, 255, 1)"} />} </div> - </div> + </button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.return ( <div className="choropleth-section" ref={showChoroplethOptionRef}> <button type="button" className="icon-rest virtualization-icon" onClick={() => setShowChoroplethOptions((previous) => !previous)} style={{display:"flex", background: "none", border: "none", width: "100%"}} > <p>{t("VISUALIZATIONS")}</p> <div className="icon"> {DigitSvgs.FilterAlt && <DigitSvgs.FilterAlt width={"1.667rem"} height={"1.667rem"} fill={"rgba(255, 255, 255, 1)"} />} </div> </button>🧰 Tools
🪛 Biome
[error] 30-30: The HTML element div is non-interactive. Do not use tabIndex.
Adding non-interactive elements to the keyboard navigation flow can confuse users.
Unsafe fix: Remove the tabIndex attribute.(lint/a11y/noNoninteractiveTabindex)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FilterSection.js (4)
1-8: 🧹 Nitpick (assertive)
Add PropTypes validation for better type safety and documentation.
While the component props are well-structured, adding PropTypes validation would improve type safety and serve as documentation for component usage.
Add the following at the end of the file:
import PropTypes from 'prop-types'; FilterSection.propTypes = { filterProperties: PropTypes.arrayOf(PropTypes.string).isRequired, showFilterOptionRef: PropTypes.object.isRequired, showFilterOptions: PropTypes.bool.isRequired, setShowFilterOptions: PropTypes.func.isRequired, filterSelections: PropTypes.arrayOf(PropTypes.string).isRequired, setFilterSelections: PropTypes.func.isRequired, t: PropTypes.func.isRequired };
22-35:
⚠️ Potential issueImprove accessibility and styling practices.
The current implementation has accessibility issues:
- Using a non-interactive div with tabIndex is not recommended
- Keyboard handling is incomplete
Inline styles could be moved to CSS classes for better maintainability.
Apply these changes:
- <div className="filter-section" ref={showFilterOptionRef}> - <div - className="icon-rest filter-icon" - onClick={() => setShowFilterOptions((previous) => !previous)} - onKeyUp={() => setShowFilterOptions((previous) => !previous)} - tabIndex={0} - style={{display:"flex"}} + <div className="filter-section" ref={showFilterOptionRef}> + <button + className="icon-rest filter-icon filter-button" + onClick={() => setShowFilterOptions((previous) => !previous)} + aria-expanded={showFilterOptions} + aria-controls="filter-options"Add to your CSS:
.filter-button { display: flex; background: none; border: none; cursor: pointer; padding: 0; }🧰 Tools
🪛 Biome
[error] 28-28: The HTML element div is non-interactive. Do not use tabIndex.
Adding non-interactive elements to the keyboard navigation flow can confuse users.
Unsafe fix: Remove the tabIndex attribute.(lint/a11y/noNoninteractiveTabindex)
56-74: 🛠️ Refactor suggestion
Move styles to CSS and add loading state.
The button implementation has several areas for improvement:
- Extensive inline styles should be moved to CSS classes
- Consider adding loading state during filter clearing
Suggested implementation:
<Button variation="secondary" - textStyles={{ width: "fit-content", fontSize: "0.875rem", fontWeight: "600", display: "flex", alignItems: "center" }} className="button-primary clear-filters-button" - style={{ - width: "100%", - display: "flex", - alignItems: "center", - justifyContent: "flex-start", - border: 0, - padding: "0 0.7rem 0 0.7rem", - height: "2.5rem", - maxHeight: "2.5rem", - backgroundColor: "rgba(250, 250, 250, 1)", - }} icon={"AutoRenew"} label={t("CLEAR_ALL_FILTERS")} + disabled={isClearing} onClick={() => { + setIsClearing(true); setFilterSelections([]); + setIsClearing(false); }} />Add to your CSS:
.clear-filters-button { width: 100%; display: flex; align-items: center; justify-content: flex-start; border: 0; padding: 0 0.7rem; height: 2.5rem; max-height: 2.5rem; background-color: rgba(250, 250, 250, 1); font-size: 0.875rem; font-weight: 600; }
36-55: 🧹 Nitpick (assertive)
Enhance accessibility and simplify styling approach.
The checkbox implementation could be improved:
- Add proper ARIA labels for screen readers
- Consolidate multiple className props into a single CSS class using composition
Consider this approach:
{filterProperties?.map((item) => ( <div id={item} key={item} className="custom-box"> <CheckBox onChange={(e) => handleChange(e, item)} label={t(item)} checked={!!filterSelections.includes(item)} - mainClassName="mainClassName" - labelClassName="labelClassName" - inputWrapperClassName="inputWrapperClassName" - inputClassName="inputClassName" - inputIconClassname="inputIconClassname" + className="filter-checkbox" + aria-label={`${t("FILTER_BY")} ${t(item)}`} iconFill={"#C84C0E"} onLabelClick={(e) => handleChange(e, item)} /> </div>Committable suggestion skipped: line range outside the PR's diff.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/ChooseActivity.js (2)
95-98: 🧹 Nitpick (assertive)
Consider improving maintainability
To enhance maintainability and reduce duplication, consider:
- Extracting the role and status arrays into constants:
const MAP_VIEW_ROLES = ["POPULATION_DATA_APPROVER", "ROOT_POPULATION_DATA_APPROVER", /* ... */]; const MAP_VIEW_STATUSES = ["RESOURCE_ESTIMATION_IN_PROGRESS", "RESOURCE_ESTIMATIONS_APPROVED"];
- Creating a helper function to determine the card state:
const getMapViewCardState = () => { const isDisabled = isCardDisabled(MAP_VIEW_ROLES, workflowData, MAP_VIEW_STATUSES); return { isDisabled, fill: isDisabled ? "#C5C5C5" : "#C84C0E" }; };Would you like me to provide a complete refactoring suggestion?
🧰 Tools
🪛 Biome
[error] 97-97: Did you mean to use a
:? An=can only follow a property name when the containing object literal is part of a destructuring pattern.(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"POPULATION_DATA_APPROVER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"ROOT_POPULATION_DATA_APPROVER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"FACILITY_CATCHMENT_MAPPER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"ROOT_FACILITY_CATCHMENT_MAPPER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"MICROPLAN_VIEWER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"PLAN_ESTIMATION_APPROVER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"ROOT_PLAN_ESTIMATION_APPROVER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"RESOURCE_ESTIMATION_IN_PROGRESS"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"RESOURCE_ESTIMATIONS_APPROVED"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected a function body but instead found '?'.
Expected a function body here.
(parse)
[error] 97-97: expected
,but instead found:Remove :
(parse)
[error] 97-97: Expected an expression but instead found '>'.
Expected an expression here.
(parse)
[error] 97-97: Expected an expression but instead found ','.
Expected an expression here.
(parse)
95-98:
⚠️ Potential issueFix duplicate icon properties and syntax error
There are two issues in the GEOSPATIAL_MAP_VIEW card configuration:
The card has duplicate icon definitions:
- One with
iconproperty using<SVG.LocationOn>- Another with
fillproperty attempting to define the same iconThere's a syntax error in the
fillproperty definition.Apply this diff to fix the issues:
{ name: t("GEOSPATIAL_MAP_VIEW"), link: `map-view?campaignId=${campaignId}µplanId=${microplanId}`, - icon: <SVG.LocationOn height="36" width="36" fill={isCardDisabled([], workflowData) ? "#C5C5C5" : "#C84C0E"} />, - fill={isCardDisabled(["POPULATION_DATA_APPROVER","ROOT_POPULATION_DATA_APPROVER","FACILITY_CATCHMENT_MAPPER","ROOT_FACILITY_CATCHMENT_MAPPER","MICROPLAN_VIEWER","PLAN_ESTIMATION_APPROVER", "ROOT_PLAN_ESTIMATION_APPROVER"], workflowData, ["RESOURCE_ESTIMATION_IN_PROGRESS","RESOURCE_ESTIMATIONS_APPROVED"]) ? "#C5C5C5" : "#C84C0E"}/>, + icon: <SVG.LocationOn height="36" width="36" fill={isCardDisabled( + ["POPULATION_DATA_APPROVER","ROOT_POPULATION_DATA_APPROVER","FACILITY_CATCHMENT_MAPPER","ROOT_FACILITY_CATCHMENT_MAPPER","MICROPLAN_VIEWER","PLAN_ESTIMATION_APPROVER", "ROOT_PLAN_ESTIMATION_APPROVER"], + workflowData, + ["RESOURCE_ESTIMATION_IN_PROGRESS","RESOURCE_ESTIMATIONS_APPROVED"] + ) ? "#C5C5C5" : "#C84C0E"} />, disable: isCardDisabled(["POPULATION_DATA_APPROVER","ROOT_POPULATION_DATA_APPROVER","FACILITY_CATCHMENT_MAPPER","ROOT_FACILITY_CATCHMENT_MAPPER","MICROPLAN_VIEWER","PLAN_ESTIMATION_APPROVER", "ROOT_PLAN_ESTIMATION_APPROVER"], workflowData, ["RESOURCE_ESTIMATION_IN_PROGRESS","RESOURCE_ESTIMATIONS_APPROVED"]), optionKey: "GEOSPATIAL_MAP_VIEW" }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.link: `map-view?campaignId=${campaignId}µplanId=${microplanId}`, icon: <SVG.LocationOn height="36" width="36" fill={isCardDisabled( ["POPULATION_DATA_APPROVER","ROOT_POPULATION_DATA_APPROVER","FACILITY_CATCHMENT_MAPPER","ROOT_FACILITY_CATCHMENT_MAPPER","MICROPLAN_VIEWER","PLAN_ESTIMATION_APPROVER", "ROOT_PLAN_ESTIMATION_APPROVER"], workflowData, ["RESOURCE_ESTIMATION_IN_PROGRESS","RESOURCE_ESTIMATIONS_APPROVED"] ) ? "#C5C5C5" : "#C84C0E"} />, disable: isCardDisabled(["POPULATION_DATA_APPROVER","ROOT_POPULATION_DATA_APPROVER","FACILITY_CATCHMENT_MAPPER","ROOT_FACILITY_CATCHMENT_MAPPER","MICROPLAN_VIEWER","PLAN_ESTIMATION_APPROVER", "ROOT_PLAN_ESTIMATION_APPROVER"], workflowData, ["RESOURCE_ESTIMATION_IN_PROGRESS","RESOURCE_ESTIMATIONS_APPROVED"]),🧰 Tools
🪛 Biome
[error] 97-97: Did you mean to use a
:? An=can only follow a property name when the containing object literal is part of a destructuring pattern.(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"POPULATION_DATA_APPROVER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"ROOT_POPULATION_DATA_APPROVER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"FACILITY_CATCHMENT_MAPPER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"ROOT_FACILITY_CATCHMENT_MAPPER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"MICROPLAN_VIEWER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"PLAN_ESTIMATION_APPROVER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"ROOT_PLAN_ESTIMATION_APPROVER"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"RESOURCE_ESTIMATION_IN_PROGRESS"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected an identifier, an object pattern, an array pattern, or a rest pattern but instead found '"RESOURCE_ESTIMATIONS_APPROVED"'.
Expected an identifier, an object pattern, an array pattern, or a rest pattern here.
(parse)
[error] 97-97: Expected a function body but instead found '?'.
Expected a function body here.
(parse)
[error] 97-97: expected
,but instead found:Remove :
(parse)
[error] 97-97: Expected an expression but instead found '>'.
Expected an expression here.
(parse)
[error] 97-97: Expected an expression but instead found ','.
Expected an expression here.
(parse)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js (1)
226-226: 🛠️ Refactor suggestion
Consider passing required props to MapViewComponent
The MapViewComponent is rendered without any props, while it might need access to:
- hierarchyType from parent
- hierarchyData for boundary rendering
- tenantId for data fetching
Consider updating the route to include necessary props:
-<PrivateRoute path={`${path}/map-view`} component={() => <MapViewComponent />} /> +<PrivateRoute + path={`${path}/map-view`} + component={() => ( + <MapViewComponent + hierarchyType={BOUNDARY_HIERARCHY_TYPE} + hierarchyData={hierarchyData} + tenantId={tenantId} + /> + )} +/>Committable suggestion skipped: line range outside the PR's diff.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/processHierarchyAndData.js (9)
99-105: 🛠️ Refactor suggestion
Add input validation to createDataObject function
The function should validate its inputs to prevent runtime errors.
+/** + * Creates a data object with key-value pairs from headers and row data + * @param {Array} headers - Array of header names + * @param {Array} row - Array of row values + * @returns {Object} Object with header-value pairs + */ const createDataObject = (headers, row) => { + if (!Array.isArray(headers) || !Array.isArray(row)) { + throw new Error('Headers and row must be arrays'); + } + if (headers.length !== row.length) { + throw new Error('Headers and row must have the same length'); + } const dataObject = {}; headers.forEach((header, index) => { + if (header) { // Only create properties for valid headers dataObject[header] = row[index]; + } }); return dataObject; };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Creates a data object with key-value pairs from headers and row data * @param {Array} headers - Array of header names * @param {Array} row - Array of row values * @returns {Object} Object with header-value pairs */ const createDataObject = (headers, row) => { if (!Array.isArray(headers) || !Array.isArray(row)) { throw new Error('Headers and row must be arrays'); } if (headers.length !== row.length) { throw new Error('Headers and row must have the same length'); } const dataObject = {}; headers.forEach((header, index) => { if (header) { // Only create properties for valid headers dataObject[header] = row[index]; } }); return dataObject; };
81-96:
⚠️ Potential issueAdd safety checks and handle edge cases in merge function
The merge function has potential issues:
- Missing null checks before accessing properties
- No handling for circular references
- Undeclared lodash dependency
Add the following improvements:
+import _ from 'lodash'; // Add at the top of the file const mergeHierarchicalData = (data1, data2) => { + if (!data1 || !data2) return data1 || data2; + for (const [key, value] of Object.entries(data2)) { if (!data1[key]) { if (!value.data) value.data = {}; data1[key] = value || {}; } else { - data1[key].data = value.data; // Merge data + data1[key].data = value?.data || {}; // Merge data with null check mergeHierarchicalData(data1[key].children, value.children); // Recursively merge children } if (data1[key].data?.feature) { const { feature, ...temp } = value.data ? _.cloneDeep(value.data) : {}; - data1[key].data.feature.properties = { ...data1[key].data?.feature?.properties, ...temp }; + data1[key].data.feature.properties = { + ...(data1[key].data?.feature?.properties || {}), + ...temp + }; } } return data1; };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const mergeHierarchicalData = (data1, data2) => { if (!data1 || !data2) return data1 || data2; for (const [key, value] of Object.entries(data2)) { if (!data1[key]) { if (!value.data) value.data = {}; data1[key] = value || {}; } else { data1[key].data = value?.data || {}; // Merge data with null check mergeHierarchicalData(data1[key].children, value.children); // Recursively merge children } if (data1[key].data?.feature) { const { feature, ...temp } = value.data ? _.cloneDeep(value.data) : {}; data1[key].data.feature.properties = { ...(data1[key].data?.feature?.properties || {}), ...temp }; } } return data1; };
108-127: 🧹 Nitpick (assertive)
Improve findParent function implementation
The function has several areas for improvement:
- Use const instead of let for immutable variables
- Simplify the logic and reduce nesting
- Consider using immutable patterns
export const findParent = (name, hierarchy, parent, accumulator = []) => { if (!name || !hierarchy) return null; - for (let key in hierarchy) { + for (const key in hierarchy) { if (hierarchy[key]?.name == name) { accumulator.push(parent); } if (hierarchy[key]?.children) { - let response = findParent(name, hierarchy[key]?.children, hierarchy[key], accumulator); + const response = findParent(name, hierarchy[key]?.children, hierarchy[key], accumulator); if (response) response.forEach((item) => { if (!accumulator.includes(item)) { accumulator.push(item); } }); - } else { - return accumulator; } } return accumulator; };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const findParent = (name, hierarchy, parent, accumulator = []) => { if (!name || !hierarchy) return null; for (const key in hierarchy) { if (hierarchy[key]?.name == name) { accumulator.push(parent); } if (hierarchy[key]?.children) { const response = findParent(name, hierarchy[key]?.children, hierarchy[key], accumulator); if (response) response.forEach((item) => { if (!accumulator.includes(item)) { accumulator.push(item); } }); } } return accumulator; };🧰 Tools
🪛 Biome
[error] 110-110: This let declares a variable that is only assigned once.
'key' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 115-115: This let declares a variable that is only assigned once.
'response' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
172-230: 🛠️ Refactor suggestion
Refactor fetchDropdownValues for better maintainability
The function is too complex and has multiple responsibilities. It needs:
- Breaking down into smaller, focused functions
- Explicit lodash import
- Better error handling and input validation
Consider breaking down the function into smaller, more focused functions:
+import _ from 'lodash'; + +const validateInputs = (boundaryData, hierarchy, boundarySelections) => { + return !( + !hierarchy || + !boundaryData || + !boundarySelections || + hierarchy.length === 0 || + Object.keys(hierarchy).length === 0 || + Object.keys(boundaryData).length === 0 + ); +}; + +const handleInitialHierarchy = (hierarchy, boundaryData) => { + const result = _.cloneDeep(hierarchy); + result[0].dropDownOptions = findByBoundaryType( + result[0].boundaryType, + Object.values(boundaryData)[0].hierarchicalData + ).map(data => ({ + name: data, + code: data, + boundaryType: result[0].boundaryType, + parentBoundaryType: undefined, + })); + return result; +}; export const fetchDropdownValues = (boundaryData, hierarchy, boundarySelections, changedBoundaryType) => { - if (!hierarchy || !boundaryData /* ... */) return []; + if (!validateInputs(boundaryData, hierarchy, boundarySelections)) return []; - let TempHierarchy = _.cloneDeep(hierarchy); + const tempHierarchy = _.cloneDeep(hierarchy); if (!boundarySelections || Object.values(boundarySelections)?.every((item) => item?.length === 0)) { - for (let i in TempHierarchy) { - if (i === "0") { - TempHierarchy[0].dropDownOptions = /* ... */; - } else TempHierarchy[i].dropDownOptions = []; - } + return handleInitialHierarchy(tempHierarchy, boundaryData); } // ... rest of the function };Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 182-182: This let declares a variable that is only assigned once.
'TempHierarchy' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 184-184: This let declares a variable that is only assigned once.
'i' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 207-207: This let declares a variable that is only assigned once.
'currentindex' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 209-209: This let declares a variable that is only assigned once.
'childIndex' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 212-212: This let declares a variable that is only assigned once.
'newDropDownValuesForChild' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 214-214: This let declares a variable that is only assigned once.
'tempStore' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
232-239: 🧹 Nitpick (assertive)
Improve findByBoundaryType function implementation
The function needs several improvements:
- Better documentation
- Simplified logic
- Proper error handling
+/** + * Finds all boundary values of a specific type in the hierarchy + * @param {string} boundaryType - The type of boundary to search for + * @param {Object} hierarchy - The hierarchy object to search in + * @returns {Array<string>} Array of boundary values of the specified type + */ const findByBoundaryType = (boundaryType, hierarchy) => { + if (!boundaryType || !hierarchy) return []; + for (const [key, value] of Object.entries(hierarchy)) { if (value?.boundaryType === boundaryType) { return Object.keys(hierarchy).filter(Boolean); } - if (value?.children) return findByBoundaryType(boundaryType, value?.children); - return []; + if (value?.children) { + const childResult = findByBoundaryType(boundaryType, value.children); + if (childResult.length > 0) return childResult; + } } return []; };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Finds all boundary values of a specific type in the hierarchy * @param {string} boundaryType - The type of boundary to search for * @param {Object} hierarchy - The hierarchy object to search in * @returns {Array<string>} Array of boundary values of the specified type */ const findByBoundaryType = (boundaryType, hierarchy) => { if (!boundaryType || !hierarchy) return []; for (const [key, value] of Object.entries(hierarchy)) { if (value?.boundaryType === boundaryType) { return Object.keys(hierarchy).filter(Boolean); } if (value?.children) { const childResult = findByBoundaryType(boundaryType, value.children); if (childResult.length > 0) return childResult; } } return []; };🧰 Tools
🪛 Biome
[error] 233-233: This let declares some variables that are only assigned once.
'key' is never reassigned.
'value' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
241-288: 🧹 Nitpick (assertive)
Clean up and improve hierarchy filtering implementation
The hierarchy filtering functions need improvements:
- Remove commented out code
- Add proper error handling
- Improve documentation and readability
+/** + * Filters the hierarchy tree based on boundary selections + * @param {Object} hierarchyTree - The original hierarchy tree + * @param {Object} boundarySelections - Selected boundaries + * @param {Array} hierarchy - Hierarchy configuration + * @returns {Object} Filtered hierarchy tree + */ const findCurrentFilteredHierarchy = (hierarchyTree, boundarySelections, hierarchy) => { + if (!hierarchyTree || !boundarySelections || !hierarchy) return {}; const newtree = constructNewHierarchyTree(hierarchy, hierarchyTree, boundarySelections); return newtree; }; const constructNewHierarchyTree = (hierarchy, oldTree, boundarySelection, level = 0) => { - // let newTree = { ...oldTree }; // Remove commented code let newTree = {}; if (!hierarchy?.[level]) return; const levelName = hierarchy[level].boundaryType; const selections = boundarySelection[levelName] || []; if (selections.length > 0) { for (const selection of selections) { const { name } = selection; if (oldTree[name]) { newTree[name] = { ...oldTree[name] }; if (oldTree[name].children) { - oldTree[name].children; // Remove this line const nonNullObject = Object.entries(oldTree[name].children) .reduce((acc, [key, value]) => { if (value.name !== null) { acc[key] = value; } return acc; }, {}); newTree[name].children = constructNewHierarchyTree( hierarchy, nonNullObject, boundarySelection, level + 1 ); } } } } else { const nonNullObject = Object.entries(oldTree) .reduce((acc, [key, value]) => { if (value.name !== null) { acc[key] = value; } return acc; }, {}); newTree = nonNullObject; } return newTree; };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Filters the hierarchy tree based on boundary selections * @param {Object} hierarchyTree - The original hierarchy tree * @param {Object} boundarySelections - Selected boundaries * @param {Array} hierarchy - Hierarchy configuration * @returns {Object} Filtered hierarchy tree */ const findCurrentFilteredHierarchy = (hierarchyTree, boundarySelections, hierarchy) => { if (!hierarchyTree || !boundarySelections || !hierarchy) return {}; const newtree = constructNewHierarchyTree(hierarchy, hierarchyTree, boundarySelections); return newtree; }; const constructNewHierarchyTree = (hierarchy, oldTree, boundarySelection, level = 0) => { let newTree = {}; if (!hierarchy?.[level]) return; const levelName = hierarchy[level].boundaryType; const selections = boundarySelection[levelName] || []; if (selections.length > 0) { for (const selection of selections) { const { name } = selection; if (oldTree[name]) { newTree[name] = { ...oldTree[name] }; if (oldTree[name].children) { const nonNullObject = Object.entries(oldTree[name].children) .reduce((acc, [key, value]) => { if (value.name !== null) { acc[key] = value; } return acc; }, {}); newTree[name].children = constructNewHierarchyTree( hierarchy, nonNullObject, boundarySelection, level + 1 ); } } } } else { const nonNullObject = Object.entries(oldTree) .reduce((acc, [key, value]) => { if (value.name !== null) { acc[key] = value; } return acc; }, {}); newTree = nonNullObject; } return newTree; };
135-169: 🛠️ Refactor suggestion
Improve findChildren function readability and maintainability
The function needs several improvements:
- Better documentation of the algorithm
- More descriptive variable names
- Simplified logic structure
+/** + * Finds all children nodes for given parent nodes in a hierarchy + * @param {Array<string>} parents - Array of parent node names to search for + * @param {Object} hierarchy - Hierarchical tree structure + * @returns {Object} Combined children of all parent nodes + */ export const findChildren = (parents, hierarchy) => { const hierarchyTraveller = (parents, hierarchy, accumulator = {}) => { - let tempData = []; + let collectedChildren = {}; if (accumulator && Object.keys(accumulator).length !== 0) { - tempData = { + collectedChildren = { ...accumulator, ...hierarchy.reduce((data, item) => { if (parents.includes(item?.name) && item?.children) { - for (const key in item.children) { - if (!data[key]) { - data[key] = item.children[key]; - } - } + Object.entries(item.children).forEach(([key, child]) => { + if (!data[key]) { + data[key] = child; + } + }); } return data; }, {}), }; } else { - tempData = hierarchy.reduce((data, item) => { + collectedChildren = hierarchy.reduce((data, item) => { // ... same reduction logic }, {}); } - for (let parent of hierarchy) { + for (const parent of hierarchy) { if (parent?.children) { - tempData = hierarchyTraveller(parents, Object.values(parent?.children), tempData); + collectedChildren = hierarchyTraveller( + parents, + Object.values(parent?.children), + collectedChildren + ); } } - return tempData; + return collectedChildren; }; return hierarchyTraveller(parents, Object.values(hierarchy), {}); };Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 163-163: This let declares a variable that is only assigned once.
'parent' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
290-351: 🧹 Nitpick (assertive)
Consider performance optimizations for large trees
The aggregate calculation function is well-structured but could benefit from some optimizations:
- Add type checking for numerical properties
- Consider memory optimization for large trees
- Add progress tracking for long operations
export const calculateAggregateForTree = (tree) => { try { + const isValidNumber = (value) => { + return typeof value === 'number' && !isNaN(value) && isFinite(value); + }; + function calculateAggregate(node) { if (!node.children || Object.keys(node.children).length === 0) { return { ...node, data: { ...node.data } }; } const newChildren = {}; + const childCount = Object.keys(node.children).length; + let processedCount = 0; for (const childKey in node.children) { const child = node.children[childKey]; const newChild = calculateAggregate(child); newChildren[childKey] = newChild; + processedCount++; + + // Log progress for large trees + if (childCount > 1000 && processedCount % 1000 === 0) { + console.log(`Processed ${processedCount}/${childCount} children`); + } } const aggregate = {}; for (const childKey in newChildren) { const child = newChildren[childKey]; for (const prop in child.data) { - if (typeof child.data[prop] === "number") { + if (isValidNumber(child.data[prop])) { aggregate[prop] = (aggregate[prop] || 0) + child.data[prop]; } } } const newNode = { ...node, data: { ...node.data, ...aggregate }, children: newChildren, }; if (newNode.data.feature) { newNode.data.feature.properties = { ...newNode.data.feature.properties, ...aggregate }; } return newNode; } const newTree = {}; for (const nodeKey in tree) { const node = tree[nodeKey]; const newNode = calculateAggregate(node); newTree[nodeKey] = newNode; } return newTree; } catch (error) { - console.error("Failed to calculate treenode aggregates"); + console.error("Failed to calculate treenode aggregates:", error); return {}; } };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const calculateAggregateForTree = (tree) => { try { const isValidNumber = (value) => { return typeof value === 'number' && !isNaN(value) && isFinite(value); }; function calculateAggregate(node) { if (!node.children || Object.keys(node.children).length === 0) { return { ...node, data: { ...node.data } }; } const newChildren = {}; const childCount = Object.keys(node.children).length; let processedCount = 0; for (const childKey in node.children) { const child = node.children[childKey]; const newChild = calculateAggregate(child); newChildren[childKey] = newChild; processedCount++; // Log progress for large trees if (childCount > 1000 && processedCount % 1000 === 0) { console.log(`Processed ${processedCount}/${childCount} children`); } } const aggregate = {}; for (const childKey in newChildren) { const child = newChildren[childKey]; for (const prop in child.data) { if (isValidNumber(child.data[prop])) { aggregate[prop] = (aggregate[prop] || 0) + child.data[prop]; } } } const newNode = { ...node, data: { ...node.data, ...aggregate }, children: newChildren, }; if (newNode.data.feature) { newNode.data.feature.properties = { ...newNode.data.feature.properties, ...aggregate }; } return newNode; } const newTree = {}; for (const nodeKey in tree) { const node = tree[nodeKey]; const newNode = calculateAggregate(node); newTree[nodeKey] = newNode; } return newTree; } catch (error) { console.error("Failed to calculate treenode aggregates:", error); return {}; } };
1-78: 🛠️ Refactor suggestion
Add input validation and improve code organization
The main processing function needs several improvements:
- Add input validation for the parameters
- Break down the nested loops into smaller functions
- Add JSDoc documentation for better maintainability
Here's a suggested improvement for the function signature:
+/** + * Processes hierarchy and data to create hierarchical lists and data structure + * @param {Array} hierarchy - Array of boundary type definitions + * @param {Array} allData - Array of data sets to process + * @returns {Object} Object containing hierarchyLists and hierarchicalData + * @throws {Error} When input parameters are invalid + */ -export const processHierarchyAndData = (hierarchy, allData) => { +export const processHierarchyAndData = (hierarchy, allData) => { + if (!Array.isArray(hierarchy) || !Array.isArray(allData)) { + throw new Error('Invalid input parameters: hierarchy and allData must be arrays'); + }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 18-18: This let declares a variable that is only assigned once.
'parent' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 45-45: This let declares a variable that is only assigned once.
'nextHierarchyList' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/fileData.js (2)
1-4: 🧹 Nitpick (assertive)
Consider using a more precise coordinate system for geospatial data
The current implementation uses EPSG:3857 (Web Mercator) projection which:
- Causes significant distortion at higher latitudes
- Uses floating-point coordinates that may lose precision at runtime (as indicated by static analysis)
Consider using:
- EPSG:4326 (WGS 84) for raw coordinates
- Or EPSG:7755 (Indian Coordinate System) for India-specific data
8-15:
⚠️ Potential issueFix inconsistent country data in properties
The properties contain conflicting country information:
country_nameis set to "India"properties.ADM0_ENis set to "Mozambique"This inconsistency could cause issues with data processing and visualization.
Apply this diff to fix the inconsistency:
properties: { country_name: "India", country_code: "IND", global_id: "3c1b3b06-3958-424b-a160-3e25998cd027", source: "WFP, INE", source_date: "2007-01-01", - properties: { ADM0_EN: "Mozambique", OBJECTID: 1, ADM0_PCODE: "MZ", Shape_Area: 67.0418795361, Shape_Leng: 96.6578168932 }, + properties: { ADM0_EN: "India", OBJECTID: 1, ADM0_PCODE: "IN", Shape_Area: 67.0418795361, Shape_Leng: 96.6578168932 }, population_1: 26876090.0, },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.properties: { country_name: "India", country_code: "IND", global_id: "3c1b3b06-3958-424b-a160-3e25998cd027", source: "WFP, INE", source_date: "2007-01-01", properties: { ADM0_EN: "India", OBJECTID: 1, ADM0_PCODE: "IN", Shape_Area: 67.0418795361, Shape_Leng: 96.6578168932 }, population_1: 26876090.0,health/micro-ui/web/micro-ui-internals/example/src/UICustomizations.js (1)
983-985: 🧹 Nitpick (assertive)
Consider extracting common button styles to a shared constant.
The inline style
minWidth:"240px"is repeated across multiple buttons. To maintain consistency and follow DRY principles, consider extracting this to a shared constant.+const COMMON_BUTTON_STYLES = { minWidth: "240px" }; <ButtonNew label={t("START")} variation="primary" icon={"ArrowForward"} - style={{minWidth:"240px"}} + style={COMMON_BUTTON_STYLES} type="button" isDisabled={!hasRequiredRole} onClick={(e) => onActionSelect("START", row)} />Committable suggestion skipped: line range outside the PR's diff.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/BoundaryFilter.js (2)
11-11: 🧹 Nitpick (assertive)
Use 'const' instead of 'let' in the 'for...in' loop.
The variable
keyis not reassigned within the loop. Usingconstimproves code clarity and indicates that the binding will not change.Apply this diff to update the declaration:
- for (let key in obj) { + for (const key in obj) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for (const key in obj) {🧰 Tools
🪛 Biome
[error] 11-11: This let declares a variable that is only assigned once.
'key' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
186-191: 🧹 Nitpick (assertive)
⚠️ Potential issueEnable the 'Clear All Filters' button functionality.
The
onClickhandler for the 'Clear All Filters' button is currently commented out, so the button will not respond when clicked. Please implement and enable thehandleClearAllfunction to ensure the button operates as intended.Would you like assistance in implementing the
handleClearAllfunction?Apply this diff to uncomment the
onClickprop:<Button variation="secondary" icon={"AutoRenew"} label={t("CLEAR_ALL_FILTERS")} - // onClick={handleClearAll} + onClick={handleClearAll} />Committable suggestion skipped: line range outside the PR's diff.
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/mapview.scss (7)
59-61: 🛠️ Refactor suggestion
Use theme variables instead of hard-coded color values
Hard-coded color values are used in several places, which reduces maintainability and theming flexibility. Consider replacing them with theme variables.
Examples of hard-coded colors:
- Line 59:
border-bottom: 0.063rem solid #FFFFFF;- Line 61:
color: #ffffff;- Lines 73-74:
border-leftandborder-rightwith#ccc;- Line 142:
border: 0.063rem solid rgb(0, 0, 0, 0.1);Apply this diff as an example for line 59:
- border-bottom: 0.063rem solid #FFFFFF; + border-bottom: 0.063rem solid theme(digitv2.lightTheme.paper);Please update the other instances similarly, replacing hard-coded colors with appropriate theme variables.
Also applies to: 73-74, 142-142
168-168: 🧹 Nitpick (assertive)
Ensure consistent use of theme variables
There is inconsistent usage of theme variables. In some places, SCSS variables like
$primary-theme-colorare used, while in others,theme(digitv2.lightTheme.primary)is used. For maintainability, consider standardizing the approach.For example, at line 168:
- border: 0.125rem solid theme(digitv2.lightTheme.primary) !important; + border: 0.125rem solid $primary-theme-color !important;Ensure that
$primary-theme-coloris defined appropriately or adjust as needed to maintain consistency.Also applies to: 245-245
2-2:
⚠️ Potential issueFix the typo in class selector name
In line 2, the class selector
&.leaftlet-controlcontains a typo. It should be&.leaflet-control.Apply this diff to fix the typo:
- &.leaftlet-control { + &.leaflet-control {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.&.leaflet-control {
149-153:
⚠️ Potential issueAvoid overriding margin settings
The
margin: 0;declaration at line 153 overrides the previousmargin-left: auto;andmargin-right: auto;settings, negating center alignment. To maintain the centering of thepelements, remove themargin: 0;declaration.Apply this diff to fix the issue:
margin-left: auto; margin-right: auto; - margin: 0; padding: 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.margin-left: auto; margin-right: auto; color: theme(digitv2.lightTheme.text-primary); padding: 0;
154-154: 🧹 Nitpick (assertive)
Include fallback fonts in
font-familydeclarationsAt line 154, the
font-familyproperty specifies onlyRoboto. It's good practice to include fallback fonts to ensure consistent rendering across different systems.Apply this diff to include fallback fonts:
- font-family: Roboto; + font-family: 'Roboto', sans-serif;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.font-family: 'Roboto', sans-serif;
142-142:
⚠️ Potential issueCorrect usage of RGBA color function
At line 142, the
rgb()function is used with an alpha value, which is incorrect. It should bergba()instead.Apply this diff to fix the issue:
- border: 0.063rem solid rgb(0, 0, 0, 0.1); + border: 0.063rem solid rgba(0, 0, 0, 0.1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.border: 0.063rem solid rgba(0, 0, 0, 0.1);
295-295:
⚠️ Potential issueAdd missing semicolon
In line 295, there is a missing semicolon at the end of the
colorproperty, which can cause a syntax error.Apply this diff to fix the syntax error:
- color: theme(digitv2.lightTheme.text-primary) + color: theme(digitv2.lightTheme.text-primary);Committable suggestion skipped: line range outside the PR's diff.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MapViewComponent.js (11)
256-331: 🧹 Nitpick (assertive)
Address potential issues with the loader state and commented code
The
loaderstate is initialized and updated but its usage is commented out in the JSX. This might lead to confusion about the loading state management.
- If the loading indicator is needed, consider uncommenting the related code and ensuring it functions correctly.
- If it's not required, remove the
loaderstate and associated code to clean up the component.🧰 Tools
🪛 Biome
[error] 263-263: This let declares a variable that is only assigned once.
'newLayer' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 273-273: This let declares a variable that is only assigned once.
'geojsonsBase' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 275-275: This let declares a variable that is only assigned once.
'baseLayer' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 277-277: This let declares a variable that is only assigned once.
'bounds' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 312-312: This let declares a variable that is only assigned once.
'bounds' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 317-317: This let declares a variable that is only assigned once.
'childrenGeojsonLayer' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 323-323: This let declares a variable that is only assigned once.
'filterGeojsonLayer' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
312-312: 🧹 Nitpick (assertive)
Use 'const' instead of 'let' for 'bounds'
The variable
boundsis declared withletbut is not reassigned. Useconstto indicate that it remains constant after initialization.Apply this diff:
- let bounds = findBounds(geojsons); + const bounds = findBounds(geojsons);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const bounds = findBounds(geojsons);🧰 Tools
🪛 Biome
[error] 312-312: This let declares a variable that is only assigned once.
'bounds' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
214-214: 🛠️ Refactor suggestion
Simplify boolean assignment by removing unnecessary ternary operator
Line 214 uses a ternary operator to assign a boolean value. Simplify it by directly assigning the evaluated expression.
Apply this diff:
- enabled: jurisdiction && jurisdiction.length > 0 ? true : false, + enabled: jurisdiction && jurisdiction.length > 0,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.enabled: jurisdiction && jurisdiction.length > 0,🧰 Tools
🪛 Biome
[error] 214-214: 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)
133-133: 🧹 Nitpick (assertive)
Use 'const' instead of 'let' for 'baseMaps'
The variable
baseMapsis declared withletbut is not reassigned. Usingconstcan improve code safety and readability.Apply this diff:
- let baseMaps = {}; + const baseMaps = {};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const baseMaps = {};🧰 Tools
🪛 Biome
[error] 133-133: This let declares a variable that is only assigned once.
'baseMaps' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
183-183: 🛠️ Refactor suggestion
Use 'const' instead of 'var' for 'verticalBounds'
Variable
verticalBoundsis declared withvar, which is function-scoped. Useconstto prevent unintended scope leakage and reassignment.Apply this diff:
- var verticalBounds = L.latLngBounds(L.latLng(-90, -170), L.latLng(85, 190)); + const verticalBounds = L.latLngBounds(L.latLng(-90, -170), L.latLng(85, 190));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const verticalBounds = L.latLngBounds(L.latLng(-90, -170), L.latLng(85, 190));🧰 Tools
🪛 Biome
[error] 183-183: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.(lint/style/noVar)
323-323: 🧹 Nitpick (assertive)
Use 'const' instead of 'let' for 'filterGeojsonLayer'
The variable
filterGeojsonLayeris not reassigned after declaration. Usingconstenhances code readability.Apply this diff:
- let filterGeojsonLayer = addGeojsonToMap(map, filterGeojsonWithProperties, t); + const filterGeojsonLayer = addGeojsonToMap(map, filterGeojsonWithProperties, t);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const filterGeojsonLayer = addGeojsonToMap(map, filterGeojsonWithProperties, t);🧰 Tools
🪛 Biome
[error] 323-323: This let declares a variable that is only assigned once.
'filterGeojsonLayer' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
174-174: 🧹 Nitpick (assertive)
Use 'const' instead of 'let' for 'map_i'
The variable
map_iis declared withletbut is not reassigned. Usingconstis preferred for variables that do not change.Apply this diff:
- let map_i = L.map(id, mapConfig); + const map_i = L.map(id, mapConfig);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const map_i = L.map(id, mapConfig);🧰 Tools
🪛 Biome
[error] 174-174: This let declares a variable that is only assigned once.
'map_i' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
71-72: 🛠️ Refactor suggestion
Use 'const' instead of 'var' for state variables
Variables declared with
varare function-scoped and can lead to unexpected behavior. Since these variables are not reassigned, it's better to useconst.Apply this diff to replace
varwithconst:- var [map, setMap] = useState(null); + const [map, setMap] = useState(null); - var [_mapNode, set__mapNode] = useState("map"); + const [_mapNode, set__mapNode] = useState("map");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const [map, setMap] = useState(null); const [_mapNode, set__mapNode] = useState("map");🧰 Tools
🪛 Biome
[error] 71-71: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.(lint/style/noVar)
[error] 72-72: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.(lint/style/noVar)
51-51: 🛠️ Refactor suggestion
Simplify boolean assignment by removing unnecessary ternary operator
Line 51 uses a ternary operator to assign a boolean value. You can simplify this by using the logical evaluation directly.
Apply this diff to improve the code:
- enabled: url?.campaignId ? true : false, + enabled: !!url?.campaignId,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.enabled: !!url?.campaignId,🧰 Tools
🪛 Biome
[error] 51-51: 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)
166-166: 🧹 Nitpick (assertive)
Use 'const' instead of 'let' for 'mapConfig'
The variable
mapConfigis declared withletbut is never reassigned. Changing it toconstenhances code clarity.Apply this diff:
- let mapConfig = { + const mapConfig = {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const mapConfig = {🧰 Tools
🪛 Biome
[error] 166-166: This let declares a variable that is only assigned once.
'mapConfig' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
256-256:
⚠️ Potential issueAvoid using async functions directly in useEffect
Using an async function directly in
useEffectis discouraged and can lead to unexpected behavior. Instead, define and invoke the async function within the effect.Apply this refactor:
-useEffect(async () => { +useEffect(() => { + const fetchData = async () => { // Your async code here + }; + fetchData();Committable suggestion skipped: line range outside the PR's diff.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/mappingUtils.js (11)
276-284:
⚠️ Potential issueWrap variable declarations inside switch cases with blocks
Variables declared inside
switchcases without enclosing braces may become accessible to other cases, leading to unexpected behaviors. This can cause logic errors and difficult-to-trace bugs.Wrap each case block with braces
{}to create a new scope and restrict variable access:switch (fileData?.fileType) { case EXCEL: + { const { dataAvailabilityCheck: excelDataAvailabilityCheck, hasLocationData, convertedData } = processExcelFile( fileData, latLngColumns, resources, formulaConfiguration, hypothesisAssumptionsList, schema, t ); dataAvailabilityCheck = excelDataAvailabilityCheck; // Rest of the code... + } break; case GEOJSON: case SHAPEFILE: + { const { dataAvailabilityCheck: geoJsonDataAvailabilityCheck, dataWithResources } = processGeoJsonFile( fileData, filterProperty, resources, formulaConfiguration, hypothesisAssumptionsList, t ); dataAvailabilityCheck = geoJsonDataAvailabilityCheck; // Rest of the code... + } break; default: break; }Also applies to: 295-302
🧰 Tools
🪛 Biome
[error] 276-284: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
395-395: 🛠️ Refactor suggestion
Use
Number.isNaNinstead ofisNaNThe global
isNaNfunction coerces the argument to a number before testing, which can lead to unexpected results. UseNumber.isNaNto check forNaNvalues without type coercion.Apply this diff:
- const convertedValues = values.map((item) => (!isNaN(item) ? item : 0)); + const convertedValues = values.map((item) => (!Number.isNaN(item) ? item : 0));And similarly for other occurrences:
- const percent = !isNaN(value) ? ((value - minValue) / (maxValue - minValue)) * 100 : 0; + const percent = !Number.isNaN(value) ? ((value - minValue) / (maxValue - minValue)) * 100 : 0;Also applies to: 574-574, 578-578
🧰 Tools
🪛 Biome
[error] 395-395: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
193-193: 🧹 Nitpick (assertive)
Avoid reassigning function parameters
Reassigning function parameters can lead to unintended side effects and make the code harder to understand. Instead, use local variables to store new values.
For example, in
setAvailabilityAndToastMessages, avoid reassigningdataAvailabilityCheck:- const setAvailabilityAndToastMessages = (dataAvailabilityCheck, combineList, files, setToast, t) => { + const setAvailabilityAndToastMessages = (initialDataAvailabilityCheck, combineList, files, setToast, t) => { let dataAvailabilityCheck = initialDataAvailabilityCheck;Similarly, make adjustments in other affected functions.
Also applies to: 196-196, 224-225
🧰 Tools
🪛 Biome
[error] 193-193: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
141-141: 🛠️ Refactor suggestion
Avoid using spread syntax on accumulators
Using the spread operator (
...) on accumulators in a.reducefunction can lead to O(n²) time complexity, which impacts performance. Consider using methods likeObject.assignor directly modifying the accumulator.Refactor the code to improve performance.
For example, in line 141:
- const properties = item[0].reduce((acc, cell, index) => ({ ...acc, [cell]: row[index] }), {}); + const properties = item[0].reduce((acc, cell, index) => { + acc[cell] = row[index]; + return acc; + }, {});And similarly in line 173.
Also applies to: 173-173
🧰 Tools
🪛 Biome
[error] 141-141: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
703-703: 🧹 Nitpick (assertive)
Simplify conditional expressions
The ternary operator in the condition can be simplified for better readability.
Apply this diff:
- return nonePresent ? true : allPresent ? true : false; + return nonePresent || allPresent;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.return nonePresent || allPresent;🧰 Tools
🪛 Biome
[error] 703-703: 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)
596-599: 🧹 Nitpick (assertive)
Replace
varwithletorconstfor variable declarationsUsing
varcan lead to unexpected behavior due to its function scope and hoisting. It's recommended to useletorconstfor block-scoped variables.Apply this diff:
- var minLat = Number.MAX_VALUE; - var maxLat = -Number.MAX_VALUE; - var minLng = Number.MAX_VALUE; - var maxLng = -Number.MAX_VALUE; + let minLat = Number.MAX_VALUE; + let maxLat = -Number.MAX_VALUE; + let minLng = Number.MAX_VALUE; + let maxLng = -Number.MAX_VALUE;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let minLat = Number.MAX_VALUE; let maxLat = -Number.MAX_VALUE; let minLng = Number.MAX_VALUE; let maxLng = -Number.MAX_VALUE;🧰 Tools
🪛 Biome
[error] 596-596: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'let' instead.(lint/style/noVar)
[error] 597-597: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'let' instead.(lint/style/noVar)
[error] 598-598: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'let' instead.(lint/style/noVar)
[error] 599-599: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'let' instead.(lint/style/noVar)
712-717: 🧹 Nitpick (assertive)
Use
constfor variables that are not reassignedVariables that are not reassigned after their initial assignment should be declared with
constto indicate that their value remains constant throughout the scope.Apply this diff:
- for (let [key, value] of Object.entries(obj)) { + for (const [key, value] of Object.entries(obj)) {And similarly use
constinstead ofletforkeysin line 711.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for (const [key, value] of Object.entries(obj)) { keys.push(key); if (value.children) { getAllKeys(value.children, keys); } }🧰 Tools
🪛 Biome
[error] 712-712: This let declares some variables that are only assigned once.
'key' is never reassigned.
'value' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
693-693: 🧹 Nitpick (assertive)
Avoid assignments within expressions
The assignment within the expression can be confusing and hard to read. It's better to perform the assignment separately.
Apply this diff:
- Object.values(boundarySelections).forEach((item) => (selectionList = [...selectionList, ...item.map((e) => e.name)])); + Object.values(boundarySelections).forEach((item) => { + selectionList = [...selectionList, ...item.map((e) => e.name)]; + });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Object.values(boundarySelections).forEach((item) => { selectionList = [...selectionList, ...item.map((e) => e.name)]; });🧰 Tools
🪛 Biome
[error] 693-693: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
34-37: 🧹 Nitpick (assertive)
Use
constinstead ofletfor variables that are not reassignedThe variables
newObject,key,value, andaggregatedTreeare not reassigned after their initial assignment. Declare them withconstto improve code clarity.Apply this diff:
- let newObject = {}; + const newObject = {}; for (let [key, value] of Object.entries(entity)) { if (!value?.["hierarchicalData"]) continue; - let aggregatedTree = calculateAggregateForTree(value?.["hierarchicalData"]); + const aggregatedTree = calculateAggregateForTree(value?.["hierarchicalData"]); newObject[key] = { ...value, hierarchicalData: aggregatedTree }; }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 34-34: This let declares a variable that is only assigned once.
'newObject' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 35-35: This let declares some variables that are only assigned once.
'key' is never reassigned.
'value' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 37-37: This let declares a variable that is only assigned once.
'aggregatedTree' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
508-512: 🧹 Nitpick (assertive)
Simplify string concatenation using template literals
Using template literals improves readability and makes string interpolation cleaner.
Apply this diff:
- popupContent += - "<tr style='padding-top:0.5rem;'><td style='padding-top:0.5rem; font-family: Roboto;font-size: 0.8rem;font-weight: 700;text-align: left; color:rgba(80, 90, 95, 1);padding-right:1rem'>" + - t(prop) + - "</td><td>" + - data + - "</td></tr>"; + popupContent += ` + <tr style='padding-top:0.5rem;'> + <td style='padding-top:0.5rem; font-family: Roboto;font-size: 0.8rem;font-weight: 700;text-align: left; color:rgba(80, 90, 95, 1);padding-right:1rem'> + ${t(prop)} + </td> + <td>${data}</td> + </tr>`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.popupContent += ` <tr style='padding-top:0.5rem;'> <td style='padding-top:0.5rem; font-family: Roboto;font-size: 0.8rem;font-weight: 700;text-align: left; color:rgba(80, 90, 95, 1);padding-right:1rem'> ${t(prop)} </td> <td>${data}</td> </tr>`;🧰 Tools
🪛 Biome
[error] 508-512: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
11-17:
⚠️ Potential issueFix incorrect tile coordinate calculation in
generatePreviewUrlThe calculation of
lonandlatin thegeneratePreviewUrlfunction is incorrect. Specifically,Math.pow(0, zoom)will always return0for anyzoomlevel greater than0, which results inlonbeing0. This will lead to incorrect tile coordinates and prevent the map preview URL from generating correctly.Apply this diff to correct the calculation:
export const generatePreviewUrl = (baseMapUrl, center = [0, 0], zoom = 5) => { - const lon = Math.floor(((center[1] + 180) / 360) * Math.pow(0, zoom)); + const xTile = Math.floor(((center[1] + 180) / 360) * Math.pow(2, zoom)); const lat = Math.floor( ((1 - Math.log(Math.tan((center[0] * Math.PI) / 180) + 1 / Math.cos((center[0] * Math.PI) / 180)) / Math.PI) / 2) * Math.pow(2, zoom) ); if (baseMapUrl) { - return baseMapUrl.replace("{z}", zoom).replace("{x}", lat).replace("{y}", lon); + return baseMapUrl.replace("{z}", zoom).replace("{x}", xTile).replace("{y}", lat); } // Return a default preview URL or handle this case as needed return "default-preview-url.jpg"; // todo };Ensure that you also update
latand use appropriate variable names (xTileandyTile) to reflect their purposes.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const xTile = Math.floor(((center[1] + 180) / 360) * Math.pow(2, zoom)); const lat = Math.floor( ((1 - Math.log(Math.tan((center[0] * Math.PI) / 180) + 1 / Math.cos((center[0] * Math.PI) / 180)) / Math.PI) / 2) * Math.pow(2, zoom) ); if (baseMapUrl) { return baseMapUrl.replace("{z}", zoom).replace("{x}", xTile).replace("{y}", lat); } // Return a default preview URL or handle this case as needed return "default-preview-url.jpg"; // todohealth/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss (1)
2766-2771: 🧹 Nitpick (assertive)
Remove commented-out code for codebase cleanliness
The code between lines 2766-2771 is commented out. Keeping obsolete or unused code can clutter the codebase and make maintenance more challenging. If this code is no longer needed, consider removing it to enhance code readability.
Apply this diff to remove the commented-out code:
- /*.digit-dropdown-employee-select-wrap{ - width: 27rem; - } - .digit-text-input-field{ - width: 27rem; - }*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/ChooseActivity.js (1)
Line range hint
1-1: Consider adding error boundaries and TypeScript.The component handles complex async operations and state management. To improve reliability and maintainability:
- Add error boundaries to handle failed API calls gracefully
- Consider migrating to TypeScript for better type safety
- Add PropTypes or TypeScript interfaces for the component props
Would you like me to provide examples of implementing these improvements?
📜 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/ChooseActivity.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/ChooseActivity.js (1)
Pattern **/*.js: check
🔇 Additional comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/ChooseActivity.js (1)
95-97: 🧹 Nitpick (assertive)
Consider URL parameter security and role validation optimization.
The implementation has two areas that could be improved:
-
URL Parameters: The current implementation directly exposes
campaignIdandmicroplanIdin the URL. While this is common practice, ensure these IDs are properly validated in the target component to prevent unauthorized access. -
Role and Status Validation: The disable condition contains a long list of roles and statuses, making it harder to maintain.
Consider these improvements:
+ // Constants at the top of the file
+ const MAP_VIEW_ROLES = ["POPULATION_DATA_APPROVER", "ROOT_POPULATION_DATA_APPROVER",
+ "FACILITY_CATCHMENT_MAPPER", "ROOT_FACILITY_CATCHMENT_MAPPER", "MICROPLAN_VIEWER",
+ "PLAN_ESTIMATION_APPROVER", "ROOT_PLAN_ESTIMATION_APPROVER"];
+ const MAP_VIEW_STATUSES = ["RESOURCE_ESTIMATION_IN_PROGRESS", "RESOURCE_ESTIMATIONS_APPROVED"];
{
name: t("GEOSPATIAL_MAP_VIEW"),
- link: `map-view?campaignId=${campaignId}µplanId=${microplanId}`,
+ link: `map-view?campaignId=${encodeURIComponent(campaignId)}µplanId=${encodeURIComponent(microplanId)}`,
- icon: <SVG.LocationOn height="36" width="36" fill={isCardDisabled(["POPULATION_DATA_APPROVER","ROOT_POPULATION_DATA_APPROVER","FACILITY_CATCHMENT_MAPPER","ROOT_FACILITY_CATCHMENT_MAPPER","MICROPLAN_VIEWER","PLAN_ESTIMATION_APPROVER", "ROOT_PLAN_ESTIMATION_APPROVER"], workflowData, ["RESOURCE_ESTIMATION_IN_PROGRESS","RESOURCE_ESTIMATIONS_APPROVED"]) ? "#C5C5C5" : "#C84C0E"}/>,
- disable: isCardDisabled(["POPULATION_DATA_APPROVER","ROOT_POPULATION_DATA_APPROVER","FACILITY_CATCHMENT_MAPPER","ROOT_FACILITY_CATCHMENT_MAPPER","MICROPLAN_VIEWER","PLAN_ESTIMATION_APPROVER", "ROOT_PLAN_ESTIMATION_APPROVER"], workflowData, ["RESOURCE_ESTIMATION_IN_PROGRESS","RESOURCE_ESTIMATIONS_APPROVED"]),
+ icon: <SVG.LocationOn height="36" width="36" fill={isCardDisabled(MAP_VIEW_ROLES, workflowData, MAP_VIEW_STATUSES) ? "#C5C5C5" : "#C84C0E"}/>,
+ disable: isCardDisabled(MAP_VIEW_ROLES, workflowData, MAP_VIEW_STATUSES),
optionKey: "GEOSPATIAL_MAP_VIEW"
}Let's verify the URL parameter handling in the target component:
✅ Verification successful
Let me analyze the map-view component validation based on the search results. The key findings are:
- The map-view component is accessed via URL parameters
campaignIdandmicroplanIdinChooseActivity.js - The map-view component (
MapViewComponent.js) properly validates these parameters:- Checks if
url?.campaignIdexists before enabling campaign data fetch - Validates jurisdiction and boundary data before rendering
- Uses proper error handling and loading states
- Checks if
- The component has role-based access control through
isCardDisabled()function that checks user roles and workflow states
The URL parameters are properly validated and the component has appropriate access controls in place.
The review comment suggesting URL parameter security concerns can be marked as incorrect since:
- The IDs in URLs are standard practice for React Router navigation
- The component already implements proper validation and role-based access controls
- The target component (
MapViewComponent.js) validates the parameters before using them - The workflow state validation provides an additional layer of security
[incorrect_review_comment]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the map-view component properly validates URL parameters
# Search for parameter validation in the map view component
rg -A 5 "useQueryParams|campaignId|microplanId" --type js
Length of output: 228743
Choose the appropriate template for your PR:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores