Fix AQI indexing on charts #2248
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve significant updates to the handling of air quality data within the Charts and Modal components. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
New next-platform changes available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
platform/src/common/components/Modal/dataDownload/components/Footer.jsx (1)
59-59: Consider adding prop documentation for clarity.The button text handling looks good and provides better flexibility. To improve maintainability, consider adding JSDoc or PropTypes documentation for the
btnTextprop to clarify its usage during loading states.Footer.propTypes = { btnText: PropTypes.string, // ... other props }; /** * @param {Object} props * @param {string} [props.btnText='Download'] - Button text that persists during loading state */platform/src/common/components/Modal/dataDownload/components/CustomFields.jsx (2)
8-8: Good defensive programming approach with the type check!The addition of the type check before calling
toLowerCase()prevents potential runtime errors. However, consider handling null/undefined values explicitly for even more robustness.- if (typeof name === 'string' && name.toLowerCase() === 'airqo') { + if (!name) return ''; + if (typeof name === 'string' && name.toLowerCase() === 'airqo') {
7-11: Consider moving formatName to a shared utility fileSince this formatting logic might be relevant for consistent AQI-related naming across the application, consider:
- Moving
formatNameto a shared utility file (e.g.,src/utils/formatting.js)- Making it available for reuse in other components
- Adding more cases for other AQI-related terms if needed
This would help maintain consistent naming conventions throughout the application.
platform/src/common/components/Charts/constants/index.jsx (3)
11-60: Consider enhancing the pollutant ranges implementationA few suggestions to improve robustness and maintainability:
- The pm2_5 lower bound (12.0) differs from other pollutants (0.0). Verify if this is intentional.
- Consider adding input validation for negative values.
- Consider adding JSDoc comments to document the range meanings and units.
Example documentation:
/** * Standard AQI breakpoints for different pollutants. * @typedef {Object} PollutantRange * @property {number} limit - Upper limit of the range in μg/m³ * @property {string} category - Air quality category */ const pollutantRanges = { // ... existing code };
63-99: LGTM with suggestions for maintainabilityThe category details mapping is well-structured. Consider these optional improvements:
- Move Tailwind color classes to a separate theme constants file for better reusability.
- Consider using an enum or constants for icon names to prevent typos.
Example:
const AQI_COLORS = { GOOD: 'text-green-500', MODERATE: 'text-yellow-500', // ... etc }; const AQI_ICONS = { GOOD: 'GoodAir', MODERATE: 'Moderate', // ... etc };
Line range hint
1-101: Consider relocating AQI-specific constantsThese AQI-related constants might be useful beyond just the Charts component. Consider moving them to a more general location like
common/constants/aqi.jsto promote reusability across the application.platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx (1)
100-109: Consider synchronizing state updatesThe form reset logic is comprehensive, but the asynchronous nature of
setClearSelected(false)via setTimeout could potentially lead to race conditions with the synchronoussetFormData.Consider this more robust approach:
- setFormData({ - title: { name: 'Untitled Report' }, - network: NETWORK_OPTIONS[0] || { id: '', name: 'Default Network' }, - dataType: DATA_TYPE_OPTIONS[0], - pollutant: POLLUTANT_OPTIONS[0], - duration: null, - frequency: FREQUENCY_OPTIONS[0], - fileType: FILE_TYPE_OPTIONS[0], - }); - setTimeout(() => setClearSelected(false), 0); + Promise.all([ + setFormData({ + title: { name: 'Untitled Report' }, + network: NETWORK_OPTIONS[0] || { id: '', name: 'Default Network' }, + dataType: DATA_TYPE_OPTIONS[0], + pollutant: POLLUTANT_OPTIONS[0], + duration: null, + frequency: FREQUENCY_OPTIONS[0], + fileType: FILE_TYPE_OPTIONS[0], + }), + new Promise(resolve => setTimeout(() => { + setClearSelected(false); + resolve(); + }, 0)) + ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
platform/src/common/components/Charts/components/index.jsx(2 hunks)platform/src/common/components/Charts/constants/index.jsx(1 hunks)platform/src/common/components/Modal/dataDownload/components/CustomFields.jsx(1 hunks)platform/src/common/components/Modal/dataDownload/components/Footer.jsx(2 hunks)platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx(2 hunks)
🧰 Additional context used
🪛 Biome
platform/src/common/components/Charts/components/index.jsx
[error] 60-60: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
[error] 33-33: 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)
🔇 Additional comments (5)
platform/src/common/components/Modal/dataDownload/components/Footer.jsx (1)
11-12: LGTM! Clean default prop declarations.
The default values are appropriately set and follow React best practices.
platform/src/common/components/Charts/constants/index.jsx (1)
101-101: LGTM! Clean and clear exports
The named exports make the module interface explicit and maintainable.
platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx (1)
371-372: LGTM! Good UX improvement
The dynamic button text and loading state provide clear feedback to users during the download process. This is a solid improvement to the user experience.
platform/src/common/components/Charts/components/index.jsx (2)
2-2: Import statement enhances modularity
Importing pollutantRanges and categoryDetails from the constants module improves code organization and reusability.
26-29: Function documentation is clear and descriptive
The JSDoc comments accurately describe the parameters and return values, which will aid in code maintenance and collaboration.
| // Iterate through the sorted ranges to find the correct category | ||
| for (let i = 0; i < sortedRanges.length; i++) { | ||
| if (value > sortedRanges[i].limit) { | ||
| continue; // Skip if value exceeds the current limit |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary continue statement
The continue statement here is redundant since the loop will proceed to the next iteration naturally. Removing it simplifies the code.
Apply this diff to remove the unnecessary continue:
- continue; // Skip if value exceeds the current limit📝 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.
| continue; // Skip if value exceeds the current limit |
🧰 Tools
🪛 Biome
[error] 60-60: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
| */ | ||
| export const getAirQualityLevelText = (value, pollutionType) => { | ||
| const ranges = pollutantRanges[pollutionType] || []; | ||
| // Handle invalid inputs | ||
| if (typeof value !== 'number' || isNaN(value) || value < 0) { |
There was a problem hiding this comment.
Use Number.isNaN instead of global isNaN
Using Number.isNaN is safer and avoids unexpected type coercion issues associated with the global isNaN function.
Apply this diff to fix the issue:
- if (typeof value !== 'number' || isNaN(value) || value < 0) {
+ if (typeof value !== 'number' || Number.isNaN(value) || value < 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.
| if (typeof value !== 'number' || isNaN(value) || value < 0) { | |
| if (typeof value !== 'number' || Number.isNaN(value) || value < 0) { |
🧰 Tools
🪛 Biome
[error] 33-33: 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)
| // Ensure ranges are sorted descendingly | ||
| const sortedRanges = [...ranges].sort((a, b) => b.limit - a.limit); | ||
|
|
||
| // Iterate through the sorted ranges to find the correct category | ||
| for (let i = 0; i < sortedRanges.length; i++) { | ||
| if (value > sortedRanges[i].limit) { | ||
| continue; // Skip if value exceeds the current limit | ||
| } else { | ||
| // Current value is <= sortedRanges[i].limit | ||
| const category = sortedRanges[i].category; | ||
| let iconComponent = null; | ||
|
|
||
| // Map string identifiers to actual icon components | ||
| switch (category) { | ||
| case 'GoodAir': | ||
| iconComponent = GoodAirIcon; | ||
| break; | ||
| case 'ModerateAir': | ||
| iconComponent = ModerateIcon; | ||
| break; | ||
| case 'UnhealthyForSensitiveGroups': | ||
| iconComponent = UnhealthySGIcon; | ||
| break; | ||
| case 'Unhealthy': | ||
| iconComponent = UnhealthyIcon; | ||
| break; | ||
| case 'VeryUnhealthy': | ||
| iconComponent = VeryUnhealthyIcon; | ||
| break; | ||
| case 'Hazardous': | ||
| iconComponent = HazardousIcon; | ||
| break; | ||
| default: | ||
| iconComponent = null; | ||
| } | ||
|
|
||
| const { text, color } = | ||
| categoryDetails[category] || categoryDetails['Invalid']; | ||
| return { | ||
| airQualityText: text, | ||
| AirQualityIcon: iconComponent, | ||
| airQualityColor: color, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure highest category is assigned when value exceeds all limits
Currently, if the pollutant value exceeds all defined limits, the function returns "Invalid". To accurately reflect extreme pollutant values, it's better to assign the highest category (e.g., "Hazardous") in such cases.
Here's how you can adjust the logic:
// Iterate through the sorted ranges to find the correct category
for (let i = 0; i < sortedRanges.length; i++) {
if (value > sortedRanges[i].limit) {
- continue; // Skip if value exceeds the current limit
+ // Skip to the next iteration
continue;
} else {
// Current value is <= sortedRanges[i].limit
const category = sortedRanges[i].category;
let iconComponent = null;
// Map string identifiers to actual icon components
switch (category) {
case 'GoodAir':
iconComponent = GoodAirIcon;
break;
// ... other cases ...
case 'Hazardous':
iconComponent = HazardousIcon;
break;
default:
iconComponent = null;
}
const { text, color } =
categoryDetails[category] || categoryDetails['Invalid'];
return {
airQualityText: text,
AirQualityIcon: iconComponent,
airQualityColor: color,
};
}
}
- // If value exceeds all defined ranges
- const { text, icon, color } = categoryDetails['Invalid'];
+ // If value exceeds all defined ranges, assign the highest category
+ const highestCategory = sortedRanges[sortedRanges.length - 1].category;
+ let iconComponent = null;
+ // Map string identifiers to actual icon components
+ switch (highestCategory) {
+ case 'GoodAir':
+ iconComponent = GoodAirIcon;
+ break;
+ // ... other cases ...
+ case 'Hazardous':
+ iconComponent = HazardousIcon;
+ break;
+ default:
+ iconComponent = null;
+ }
+ const { text, color } = categoryDetails[highestCategory];
return {
airQualityText: text,
- AirQualityIcon: icon,
+ AirQualityIcon: iconComponent,
airQualityColor: color,
};Feel free to reach out if you'd like help implementing this change or if you have any questions.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 60-60: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
platform/src/common/components/Charts/MoreInsightsChart.jsx (2)
Line range hint
363-386: Consider reducing useMemo dependencies.The renderChart useMemo hook has a large number of dependencies which could lead to unnecessary re-renders. Consider breaking down the chart rendering logic into smaller, more focused memoized components.
Line range hint
89-102: Enhance error message specificity.The "No Data Available" message could be more specific about what data is required. Consider including the pollutant type and selected date range in the message to help users understand why data might be missing.
- <p className="text-sm mb-4 text-center"> - It seems there's no data to display for the selected criteria. Try - refreshing the chart or adjusting your filters. - </p> + <p className="text-sm mb-4 text-center"> + No {pollutantType.toUpperCase()} data available for the selected time period. Try + refreshing the chart or adjusting your date range and filters. + </p>platform/src/common/components/Charts/components/index.jsx (3)
2-2: Add JSDoc documentation for imported constantsConsider adding JSDoc documentation for
pollutantRangesandcategoryDetailsto improve code maintainability and type safety./** * @typedef {Object} PollutantRange * @property {number} limit - The upper limit for this range * @property {string} category - The category identifier */ /** * @type {Object.<string, PollutantRange[]>} */ import { pollutantRanges, categoryDetails } from '../constants';
41-49: Simplify logic with early returns and constantsThe function could be more maintainable with early returns and predefined constants.
+ const INVALID_RESULT = { + airQualityText: categoryDetails['Invalid'].text || 'Invalid', + AirQualityIcon: null, + airQualityColor: categoryDetails['Invalid'].color || '#808080' + }; const ranges = pollutantRanges[pollutionType]; - if (!ranges) { - const { text = 'Invalid', color = '#808080' } = categoryDetails['Invalid']; - return { - airQualityText: text, - AirQualityIcon: null, - airQualityColor: color, - }; - } + if (!ranges) return INVALID_RESULT;Also applies to: 51-73
116-151: Optimize performance with useMemo for mapped contentConsider memoizing the mapped content to prevent unnecessary re-renders.
+ const renderLocationDetails = React.useMemo(() => payload.map((point, index) => { const isHovered = index === activeIndex; return ( // ... existing JSX ); }) + , [payload, activeIndex]); return ( <div className="space-y-2"> - {payload.map((point, index) => { - // ... existing code - })} + {renderLocationDetails} </div> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
platform/src/common/components/Charts/MoreInsightsChart.jsx(1 hunks)platform/src/common/components/Charts/components/index.jsx(3 hunks)platform/src/common/components/Charts/constants/index.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- platform/src/common/components/Charts/constants/index.jsx
🧰 Additional context used
🪛 Biome
platform/src/common/components/Charts/components/index.jsx
[error] 32-32: 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)
🔇 Additional comments (3)
platform/src/common/components/Charts/MoreInsightsChart.jsx (2)
247-252: LGTM! Clean implementation of the pollutionType prop.
The addition of the pollutionType prop to CustomGraphTooltip enhances the tooltip's ability to display accurate AQI information based on the specific pollutant type. This change directly addresses the PR's objective of fixing AQI indexing on charts.
Line range hint 142-145: Consider adding PropTypes for WHO standard values.
The WHO_STANDARD_VALUE is derived from WHO_STANDARD_VALUES using pollutantType. Consider adding PropTypes or TypeScript interfaces to ensure type safety for these critical reference values.
platform/src/common/components/Charts/components/index.jsx (1)
Line range hint 26-165: Implementation successfully addresses AQI indexing requirements
The changes effectively handle different pollutant types and provide appropriate visual feedback through the chart components. The code is well-structured with proper error handling and user feedback mechanisms.
| if (typeof value !== 'number' || isNaN(value) || value < 0) { | ||
| const { text = 'Invalid', color = '#808080' } = categoryDetails['Invalid']; | ||
| return { | ||
| airQualityText: text, | ||
| AirQualityIcon: null, | ||
| airQualityColor: color, | ||
| }; |
There was a problem hiding this comment.
Replace global isNaN with Number.isNaN
Using Number.isNaN is safer as it avoids type coercion issues.
- if (typeof value !== 'number' || isNaN(value) || value < 0) {
+ if (typeof value !== 'number' || Number.isNaN(value) || value < 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.
| if (typeof value !== 'number' || isNaN(value) || value < 0) { | |
| const { text = 'Invalid', color = '#808080' } = categoryDetails['Invalid']; | |
| return { | |
| airQualityText: text, | |
| AirQualityIcon: null, | |
| airQualityColor: color, | |
| }; | |
| if (typeof value !== 'number' || Number.isNaN(value) || value < 0) { | |
| const { text = 'Invalid', color = '#808080' } = categoryDetails['Invalid']; | |
| return { | |
| airQualityText: text, | |
| AirQualityIcon: null, | |
| airQualityColor: color, | |
| }; |
🧰 Tools
🪛 Biome
[error] 32-32: 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)
| const CustomGraphTooltip = ({ | ||
| active, | ||
| payload, | ||
| activeIndex, | ||
| pollutionType, | ||
| }) => { |
There was a problem hiding this comment.
Add missing pollutionType to PropTypes
The pollutionType prop is used but not defined in PropTypes.
CustomGraphTooltip.propTypes = {
active: PropTypes.bool,
payload: PropTypes.array,
activeIndex: PropTypes.number,
+ pollutionType: PropTypes.string.isRequired,
};Committable suggestion skipped: line range outside the PR's diff.
|
New next-platform changes available for preview here |
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
platform/src/core/urls/analytics.js (1)
10-10: Consider adding documentation about the endpoint behaviorSince this is a significant change in the API's behavior, it would be helpful to add a comment explaining that this endpoint returns all sites regardless of their online status.
+// Returns summary data for all sites regardless of their online status export const SITES_SUMMARY_URL = `${BASE_AUTH_URL}/devices/sites/summary`;platform/src/common/components/Charts/components/index.jsx (1)
45-61: Consider optimizing the range checking logicThe current implementation iterates through ranges linearly. For better performance with large datasets, consider using binary search since the ranges are ordered.
- for (const range of ranges) { - if ( - value >= range.limit && - (!ranges[ranges.indexOf(range) - 1] || - value < ranges[ranges.indexOf(range) - 1].limit) - ) { + const findRange = (value, ranges) => { + let left = 0; + let right = ranges.length - 1; + while (left <= right) { + const mid = Math.floor((left + right) / 2); + if (value >= ranges[mid].limit && + (mid === 0 || value < ranges[mid - 1].limit)) { + return ranges[mid]; + } + if (value >= ranges[mid].limit) { + right = mid - 1; + } else { + left = mid + 1; + } + } + return null; + }; + const range = findRange(value, ranges); + if (range) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
platform/src/common/components/Charts/components/index.jsx(3 hunks)platform/src/common/components/Charts/constants/index.jsx(1 hunks)platform/src/core/urls/analytics.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- platform/src/common/components/Charts/constants/index.jsx
🧰 Additional context used
🪛 Biome
platform/src/common/components/Charts/components/index.jsx
[error] 26-26: 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)
🔇 Additional comments (5)
platform/src/core/urls/analytics.js (1)
10-10: Verify handling of offline sites in UI components
The removal of the online_status=online filter means this endpoint will now return all sites. While this provides more flexibility, please ensure that:
- UI components can handle both online and offline sites appropriately
- Performance remains acceptable with the potentially larger payload
platform/src/common/components/Charts/components/index.jsx (4)
2-2: Nice modularization of constants!
Moving pollutant ranges and category details to a separate constants file improves code organization and maintainability.
26-26: Replace global isNaN with Number.isNaN
Using Number.isNaN is safer as it avoids type coercion issues.
🧰 Tools
🪛 Biome
[error] 26-26: 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)
76-81: Add pollutionType to PropTypes
The pollutionType prop is used but not defined in PropTypes.
98-144: Well-structured tooltip UI implementation!
The tooltip implementation is clean and well-organized with:
- Clear section separation
- Consistent styling
- Responsive hover states
- Proper accessibility considerations
|
New next-platform changes available for preview here |
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
New Features
MoreInsightsChartcomponent to provide contextual information about the pollutant type.Bug Fixes
CustomFieldscomponent to prevent runtime errors.Chores
DataDownloadcomponent for improved clarity.