Analytics: Updated to Use "aqi_category" for cards #2234
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several modifications across multiple files, primarily focusing on the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
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/pages/api/proxy/analytics.js (1)
Line range hint
1-28: Consider enhancing the proxy handler with additional safeguards.While the basic proxy functionality is implemented correctly, consider these improvements for production readiness:
- Add request timeout configuration
- Implement request body validation
- Add rate limiting
- Consider extracting the tenant parameter to environment variables
Here's a suggested enhancement:
import axios from 'axios'; +import rateLimit from 'express-rate-limit'; +import { z } from 'zod'; + +// Request validation schema +const requestSchema = z.object({ + // Add your expected request body shape here +}); + +// Rate limiting middleware +const limiter = rateLimit({ + windowMs: 15 * 60 * 1000, // 15 minutes + max: 100 // limit each IP to 100 requests per windowMs +}); export default async function handler(req, res) { if (req.method !== 'POST') { return res.status(405).json({ error: 'Method Not Allowed' }); } + // Apply rate limiting + await limiter(req, res); + + // Validate request body + try { + requestSchema.parse(req.body); + } catch (error) { + return res.status(400).json({ error: 'Invalid request body' }); + } + // Set up the target URL const targetUrl = - 'https://analytics.airqo.net/api/v2/analytics/dashboard/chart/d3/data?tenant=airqo'; + `${process.env.ANALYTICS_API_URL}/api/v2/analytics/dashboard/chart/d3/data?tenant=${process.env.TENANT_ID}`; try { const response = await axios.post(targetUrl, req.body, { headers: { 'Content-Type': 'application/json', Authorization: req.headers.authorization, }, + timeout: 5000, // 5 second timeout }); res.status(response.status).json(response.data); } catch (error) { + // Log error for monitoring + console.error('Analytics proxy error:', error); + res.status(error.response?.status || 500).json({ error: 'Error while proxying request', message: error.message, }); } }platform/src/pages/api/proxy/sites.js (2)
13-17: Enhance request security and error handling.While the authorization header is properly forwarded, consider adding these security enhancements:
- Validate the authorization header presence
- Add request timeout
- Consider rate limiting for production use
Here's an improved version:
- const response = await axios.get(targetUrl, { - headers: { - Authorization: req.headers.authorization, - }, - }); + if (!req.headers.authorization) { + return res.status(401).json({ error: 'Authorization header required' }); + } + const response = await axios.get(targetUrl, { + headers: { + Authorization: req.headers.authorization, + }, + timeout: 5000, // 5 seconds timeout + });
21-24: Improve error response structure.The error handling is good but could be more informative and consistent.
Consider this enhanced error structure:
- res.status(error.response?.status || 500).json({ - error: 'Error while proxying request', - message: error.response?.data?.message || error.message, - }); + const statusCode = error.response?.status || 500; + res.status(statusCode).json({ + error: { + code: statusCode, + type: error.code || 'PROXY_ERROR', + message: 'Error while proxying request', + details: error.response?.data?.message || error.message, + } + });platform/src/common/components/AQNumberCard/index.jsx (2)
10-19: LGTM! Consider adding TypeScript or PropTypes for the mapping.The AQI category mapping is well-structured and comprehensive. It effectively bridges the API response categories with the IconMap keys.
Consider adding type safety:
const AQI_CATEGORY_MAP = { 'Good': /** @type {const} */ ('good'), 'Moderate': /** @type {const} */ ('moderate'), // ... rest of the mapping } as const;
146-148: Consider using a more robust number formatting approach.While the current implementation handles null cases well, consider using a more sophisticated number formatting approach to handle edge cases and avoid unnecessary trailing zeros.
Here's a suggested improvement:
- {typeof measurement?.[pollutantType]?.value === 'number' - ? measurement[pollutantType].value.toFixed(2) - : '--'} + {typeof measurement?.[pollutantType]?.value === 'number' + ? Number.isFinite(measurement[pollutantType].value) + ? new Intl.NumberFormat('en-US', { + minimumFractionDigits: 0, + maximumFractionDigits: 2, + }).format(measurement[pollutantType].value) + : '--' + : '--'}This approach:
- Handles infinite/NaN values
- Removes unnecessary trailing zeros
- Uses locale-aware formatting
platform/src/lib/store/services/sitesSummarySlice/index.js (2)
12-26: Align Error Handling Across EnvironmentsWhile the conditional API calls based on the environment are well-implemented, there's a discrepancy in the error handling between the development and production blocks. Standardizing the error handling will enhance consistency and maintainability.
Consider extracting the error handling into a shared function or aligning the logic to ensure both environments handle errors uniformly.
28-34: Safeguard Against Undefineddatain ProductionIn the production block, you check if
data.sitesis an array, which is great. However, ifdatais undefined or null, accessingdata.sitescould throw an error. Adding a check fordataensures robustness.You might update the condition as follows:
- if (Array.isArray(data.sites)) { + if (data && Array.isArray(data.sites)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
platform/src/common/components/AQNumberCard/IconMap.jsx(0 hunks)platform/src/common/components/AQNumberCard/index.jsx(3 hunks)platform/src/core/apis/DeviceRegistry.js(1 hunks)platform/src/lib/store/services/sitesSummarySlice/index.js(1 hunks)platform/src/pages/api/proxy/analytics.js(1 hunks)platform/src/pages/api/proxy/sites.js(1 hunks)
💤 Files with no reviewable changes (1)
- platform/src/common/components/AQNumberCard/IconMap.jsx
🔇 Additional comments (5)
platform/src/pages/api/proxy/analytics.js (1)
10-11: Verify the new analytics endpoint availability and configuration.
The URL change from platform.airqo.net to analytics.airqo.net suggests a service migration.
✅ Verification successful
Let me check the content of these files to better understand the context of the URL change.
The previous results show several files referencing the old domain and API URLs. I need to check the actual implementations to understand if they're related to this analytics endpoint change and if they need to be updated as well. This will help determine if this is an isolated change or part of a broader service migration that requires coordinated updates.
Let me check the environment variable samples to understand if there are any analytics-specific configurations.
The previous results show that the URL change is being made directly in the code. I want to verify if there are any environment variables specifically for analytics endpoints and if there are other analytics proxy implementations that might need similar updates.
Let me check the content of other analytics-related files to ensure consistency in the URL changes.
The environment files show that the platform previously used platform.airqo.net. I want to verify if other analytics-related files are also being updated to use the new analytics.airqo.net domain or if they should be using environment variables instead of hardcoded URLs.
The URL change appears to be an isolated proxy endpoint update
The codebase analysis reveals that:
- Other analytics endpoints properly use environment variables (
NEXT_PUBLIC_API_BASE_URL,BASE_ANALYTICS_URL_V2) instead of hardcoded URLs - The proxy endpoint in question is specifically for chart data with a tenant parameter, which appears to be a unique case
- The URL change from platform.airqo.net to analytics.airqo.net aligns with the microservices architecture where analytics-specific endpoints are being moved to a dedicated domain
The change is safe to proceed as it doesn't affect other parts of the system that correctly use environment variables for configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new analytics endpoint configuration and check for other occurrences
# Check for any other references to the old domain
echo "Checking for other references to platform.airqo.net..."
rg -l "platform.airqo.net"
# Check for environment variable usage for the URL
echo "Checking for potential environment variable configurations..."
rg -l "ANALYTICS_API_URL|ANALYTICS_BASE_URL|API_URL"
Length of output: 883
Script:
#!/bin/bash
# Check content of relevant files that might be affected by this change
echo "Checking website2 proxy route implementation..."
cat website2/src/app/api/proxy/route.ts
echo -e "\nChecking external service implementation..."
cat website2/src/services/externalService/index.tsx
echo -e "\nChecking for any environment variable definitions..."
fd ".env" --hidden --no-ignore
Length of output: 5540
Script:
#!/bin/bash
# Check environment variable samples for analytics configurations
echo "Checking platform env example..."
cat platform/.env.example
echo -e "\nChecking website2 env sample..."
cat website2/.env.sample
# Also check if there are any other analytics-related proxy endpoints
echo -e "\nChecking for other analytics proxy endpoints..."
fd "analytics" -e js -e ts -e tsx
Length of output: 1209
Script:
#!/bin/bash
# Check other analytics-related implementations
echo "Checking platform Analytics API implementation..."
cat platform/src/core/apis/Analytics.js
echo -e "\nChecking platform analytics URLs configuration..."
cat platform/src/core/urls/analytics.js
echo -e "\nChecking netmanager analytics configuration..."
cat netmanager/src/config/urls/analytics.js
Length of output: 4608
platform/src/pages/api/proxy/sites.js (2)
3-6: LGTM: Clean request method validation.
The method validation is implemented correctly, following REST API best practices by explicitly checking for the GET method and returning a proper 405 status code for unsupported methods.
1-26: Verify the impact on analytics functionality.
Since this endpoint is crucial for site summaries in the analytics component, we should verify its integration.
✅ Verification successful
Let me gather more information about the analytics integration.
Let me try one more search pattern to find components using this endpoint.
Proxy endpoint implementation is properly integrated and handled
The proxy endpoint is well-integrated within the application's architecture:
- Used by
sitesSummarySlicein development environment - Proper error handling exists in both the proxy endpoint and the core analytics API
- Maintains consistent interface with the main analytics API through
SITES_SUMMARY_URL
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for references to this API endpoint and verify error handling in the frontend
# Look for references to this API endpoint
echo "Checking for API endpoint usage..."
rg -l "sites/summary\?online_status=online" --type js
# Check for error handling in components using this endpoint
echo "Checking error handling in components..."
ast-grep --pattern 'catch (error) { $$$$ }'
Length of output: 345
Script:
#!/bin/bash
# Check the analytics.js file for the API endpoint usage context
echo "Checking analytics.js implementation..."
cat platform/src/core/urls/analytics.js
# Look for components that might be using this endpoint
echo -e "\nChecking for components importing analytics URLs..."
rg -l "from ['\"]\.\./core/urls/analytics" --type js
# Check for any error handling patterns in those components
echo -e "\nChecking error handling in related components..."
rg "try.*sites.*summary.*catch" --type js -A 5
Length of output: 903
Script:
#!/bin/bash
# Search for components using the SITES_SUMMARY_URL constant
echo "Checking for SITES_SUMMARY_URL usage..."
rg "SITES_SUMMARY_URL" --type js -B 2 -A 5
# Search for any direct API calls to the sites summary endpoint
echo -e "\nChecking for direct API calls..."
rg "/devices/sites/summary\?online_status=online" --type js -B 2 -A 5
# Look for components that might be using the proxy endpoint
echo -e "\nChecking for proxy endpoint usage..."
rg "/api/proxy/sites" --type js -B 2 -A 5
Length of output: 3268
platform/src/core/apis/DeviceRegistry.js (1)
63-63: LGTM! Endpoint update looks good.
The change from /readings/recent to /measurements/recent appears to be a deliberate API restructuring. The error handling remains robust, which is great! 👍
Let's verify the API endpoint consistency and usage:
✅ Verification successful
API endpoint change is consistent and properly integrated
The endpoint change from /readings/recent to /measurements/recent is properly reflected across the codebase:
- The Redux slice in
RecentMeasurementsSlice.jscorrectly uses the updated endpoint throughgetRecentMeasurements - All consuming components (Map sidebar, AQNumberCard) interact with the slice through the proper abstractions
- No lingering references to the old
/readings/recentendpoint were found - The store configuration and data flow remain intact
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old endpoint
# and verify the new endpoint usage across the codebase
# Check for any remaining references to the old endpoint
echo "Checking for old endpoint references..."
rg -l "readings/recent"
# Check for usage of the new endpoint
echo "Checking new endpoint usage..."
rg -l "measurements/recent"
# Look for potential test files that might need updating
echo "Checking for related test files..."
fd -g "*test*" -g "*spec*" | rg -l "DeviceRegistry|readings/recent|measurements/recent"
Length of output: 589
Script:
#!/bin/bash
# Let's try a more focused search for test files and related components
# Check for test files related to DeviceRegistry
echo "Checking for DeviceRegistry test files..."
fd "test|spec" | rg -l "DeviceRegistry"
# Check for components using RecentMeasurementsSlice
echo "Checking RecentMeasurementsSlice usage..."
rg -l "RecentMeasurementsSlice"
# Check for any remaining references to 'readings' in relevant files
echo "Checking 'readings' references in relevant files..."
rg "readings" platform/src/lib/store/services/deviceRegistry/RecentMeasurementsSlice.js platform/src/core/apis/DeviceRegistry.js
Length of output: 663
Script:
#!/bin/bash
# Let's examine the components that use RecentMeasurementsSlice to ensure they're aligned with the API change
echo "Checking Map sidebar implementation..."
rg -A 5 "RecentMeasurementsSlice" platform/src/common/components/Map/components/sidebar/index.jsx
echo "Checking AQNumberCard implementation..."
rg -A 5 "RecentMeasurementsSlice" platform/src/common/components/AQNumberCard/index.jsx
echo "Checking store configuration..."
rg -A 5 "RecentMeasurementsSlice" platform/src/lib/store/index.js
echo "Checking RecentMeasurementsSlice implementation..."
cat platform/src/lib/store/services/deviceRegistry/RecentMeasurementsSlice.js
Length of output: 3088
platform/src/lib/store/services/sitesSummarySlice/index.js (1)
36-44: Verify Cancellation Handling with Different API Clients
Currently, you're using axios.isCancel(error) to detect canceled requests. If getSitesSummaryApi doesn't use Axios internally (say it uses fetch instead), this check might not accurately detect cancellations in production.
Run the following script to confirm whether getSitesSummaryApi uses Axios and handles cancellations appropriately:
✅ Verification successful
The previous results show that getSitesSummaryApi uses a custom axios instance through createAxiosInstance. We need to verify how this instance is configured and if it properly supports cancellation.
Cancellation Handling is Correctly Implemented
The implementation is actually spot-on! The code uses a proper Axios instance created via createAxiosInstance(), which is built on top of the standard Axios library. Since axios.isCancel() is a built-in Axios utility, it will correctly detect cancellations from the Axios instance being used.
axiosConfig.jsconfirms the use ofaxios.create()to create the instance- The API call in
getSitesSummaryApiproperly passes thesignalparameter for cancellation support - The error handling correctly uses
axios.isCancel()to detect cancelled requests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if `getSitesSummaryApi` uses Axios and handles cancellations.
# Find the definition of `getSitesSummaryApi`.
fd 'Analytics.js' | xargs grep -n -A 5 'export.*getSitesSummaryApi'
# Check for Axios import in the same file.
fd 'Analytics.js' | xargs grep -n 'import.*axios'
# Search for usage of Axios in `getSitesSummaryApi`.
fd 'Analytics.js' | xargs grep -n -A 10 'function getSitesSummaryApi' | grep 'axios'
# Look for cancellation handling in `getSitesSummaryApi`.
fd 'Analytics.js' | xargs grep -n 'axios.isCancel'
Length of output: 513
Script:
#!/bin/bash
# Let's check the axiosConfig implementation to understand how axios is used
fd 'axiosConfig.js' | xargs cat
# Also check if there are any other files that might handle cancellation
rg -n "isCancel|CancelToken" --type js
Length of output: 2301
| const targetUrl = | ||
| 'https://analytics.airqo.net/api/v2/devices/sites/summary?online_status=online'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider environment configuration for the API URL.
The API URL is hardcoded, which could make it difficult to switch between environments (development, staging, production).
Consider using environment variables:
- const targetUrl =
- 'https://analytics.airqo.net/api/v2/devices/sites/summary?online_status=online';
+ const targetUrl = `${process.env.ANALYTICS_API_BASE_URL}/api/v2/devices/sites/summary?online_status=online`;Committable suggestion skipped: line range outside the PR's diff.
| const measurement = useMemo(() => { | ||
| return measurements.find((m) => m.site_id === site._id); | ||
| }, [measurements, site._id]); | ||
|
|
||
| // Extract aqi_category from measurement | ||
| const aqiCategory = useMemo(() => { | ||
| return measurement?.aqi_category || 'Unknown'; | ||
| }, [measurement]); | ||
|
|
||
| // Map aqi_category to status key for IconMap | ||
| const statusKey = AQI_CATEGORY_MAP[aqiCategory] || 'unknown'; | ||
|
|
||
| // Get the air quality text | ||
| const airQualityText = | ||
| aqiCategory === 'Unknown' | ||
| ? 'Air Quality is Unknown' | ||
| : `Air Quality is ${aqiCategory}`; | ||
|
|
||
| const AirQualityIcon = IconMap[status]; | ||
| // Get the corresponding icon from IconMap | ||
| const AirQualityIcon = IconMap[statusKey]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defensive checks for measurements array.
While the code is well-structured, it could benefit from additional null checks to prevent potential runtime errors.
Consider this safer implementation:
const measurement = useMemo(() => {
+ if (!Array.isArray(measurements)) return null;
return measurements.find((m) => m.site_id === site._id);
}, [measurements, site._id]);
const aqiCategory = useMemo(() => {
return measurement?.aqi_category || 'Unknown';
}, [measurement]);📝 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 measurement = useMemo(() => { | |
| return measurements.find((m) => m.site_id === site._id); | |
| }, [measurements, site._id]); | |
| // Extract aqi_category from measurement | |
| const aqiCategory = useMemo(() => { | |
| return measurement?.aqi_category || 'Unknown'; | |
| }, [measurement]); | |
| // Map aqi_category to status key for IconMap | |
| const statusKey = AQI_CATEGORY_MAP[aqiCategory] || 'unknown'; | |
| // Get the air quality text | |
| const airQualityText = | |
| aqiCategory === 'Unknown' | |
| ? 'Air Quality is Unknown' | |
| : `Air Quality is ${aqiCategory}`; | |
| const AirQualityIcon = IconMap[status]; | |
| // Get the corresponding icon from IconMap | |
| const AirQualityIcon = IconMap[statusKey]; | |
| const measurement = useMemo(() => { | |
| if (!Array.isArray(measurements)) return null; | |
| return measurements.find((m) => m.site_id === site._id); | |
| }, [measurements, site._id]); | |
| // Extract aqi_category from measurement | |
| const aqiCategory = useMemo(() => { | |
| return measurement?.aqi_category || 'Unknown'; | |
| }, [measurement]); | |
| // Map aqi_category to status key for IconMap | |
| const statusKey = AQI_CATEGORY_MAP[aqiCategory] || 'unknown'; | |
| // Get the air quality text | |
| const airQualityText = | |
| aqiCategory === 'Unknown' | |
| ? 'Air Quality is Unknown' | |
| : `Air Quality is ${aqiCategory}`; | |
| // Get the corresponding icon from IconMap | |
| const AirQualityIcon = IconMap[statusKey]; |
| if (axios.isCancel(error)) { | ||
| console.warn('Request was canceled:', error.message); | ||
| return rejectWithValue('Request was aborted.'); | ||
| } | ||
|
|
||
| console.error('Error fetching sites summary:', error.message || error); | ||
| return rejectWithValue(error.message || 'An error occurred.'); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Abstract Error Handling for Clean Code
The error handling in both the try block and the catch block has some duplication and could benefit from abstraction. This will make the code cleaner and easier to maintain.
Consider creating a utility function to handle errors uniformly across your async thunks. This not only reduces code repetition but also ensures consistent error messages and logging.
|
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 (5)
platform/src/common/components/Modal/dataDownload/components/CustomFields.jsx (2)
7-12: Consider enhancing the formatName utility for better robustnessThe function could be improved to handle more edge cases and be more reusable:
-const formatName = (name) => { +const formatName = (name) => { + if (!name) return ''; + if (typeof name !== 'string') return name; + if (name.toLowerCase() === 'airqo') { return 'AirQo'; } return name; };
48-48: Consider adding test coverage for name formatting consistencyThe name formatting is consistently applied across all UI elements. To maintain this consistency, consider adding test cases that verify the formatting behavior across different UI states (input, dropdown button, options list).
Would you like me to help generate test cases for the formatting behavior?
Also applies to: 67-67, 80-80
platform/src/common/components/Charts/ChartContainer.jsx (3)
68-68: Consider reducing the skeleton loader timeout further5 seconds might still feel long for users. Consider reducing it to 2-3 seconds for better perceived performance, or implementing a more dynamic approach based on actual data loading state.
- }, 5000); + }, 2500);
Line range hint
89-116: Enhance error handling with more specific error messagesThe error handling is good, but could be improved by:
- Adding error context to console.error
- Providing more specific error messages based on error type
} catch (error) { - console.error('Error exporting chart:', error); + console.error('Error exporting chart:', { + format, + error: error.message, + stack: error.stack + }); + const errorMessage = error.message === 'Unsupported format' + ? `Format ${format.toUpperCase()} is not supported.` + : `Failed to export chart as ${format.toUpperCase()}. Please try again.`; CustomToast({ - message: `Failed to export chart as ${format.toUpperCase()}.`, + message: errorMessage, type: 'error', });
Line range hint
82-116: Consider extracting chart export logic into a custom hookThe chart export functionality could be moved to a custom hook (e.g.,
useChartExport) to:
- Improve code reusability
- Make the component more focused
- Facilitate easier testing
Example implementation:
// hooks/useChartExport.js const useChartExport = (chartRef) => { const [loadingFormat, setLoadingFormat] = useState(null); const [downloadComplete, setDownloadComplete] = useState(null); const exportChart = useCallback(async (format) => { // ... existing export logic ... }, [chartRef]); return { exportChart, loadingFormat, downloadComplete }; };This would simplify the main component and make the export logic reusable across different chart components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
platform/src/common/components/Charts/ChartContainer.jsx(1 hunks)platform/src/common/components/Modal/dataDownload/components/CustomFields.jsx(4 hunks)
🔇 Additional comments (2)
platform/src/common/components/Modal/dataDownload/components/CustomFields.jsx (2)
32-37: Clean implementation of option handling
The handleSelect callback efficiently formats the name while preserving other option properties, and is properly memoized.
Line range hint 13-26: Verify integration with AQI category data
Given that this PR focuses on updating the use of 'aqi_category' from the API, let's verify if this component needs to handle AQI category data in its options.
OchiengPaul442
changed the title
OchiengPaul442
changed the title
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
platform/src/common/components/Map/components/MapNodes.js (3)
Line range hint
108-122: Add value validation in getAQICategory functionWhile the function handles invalid pollutants, it should also validate the value parameter to ensure it's a valid number.
export const getAQICategory = (pollutant, value) => { if (!Object.prototype.hasOwnProperty.call(markerDetails, pollutant)) { throw new Error(`Invalid pollutant: ${pollutant}`); } + if (typeof value !== 'number' || isNaN(value)) { + throw new Error(`Invalid value: ${value}`); + } const categories = markerDetails[pollutant];
Line range hint
267-275: Enhance error handling in createPopupHTMLThe error messages could be more specific to help with debugging. Consider extracting the validation into a separate function.
+const validateFeatureProperties = (feature) => { + if (!feature?.properties) { + throw new Error('Feature or feature properties are missing'); + } + if (!feature.properties.pm2_5) { + throw new Error('PM2.5 data is missing'); + } + if (!feature.properties.aqi) { + throw new Error('AQI data is missing'); + } +}; export const createPopupHTML = ({ feature, images }) => { - if (!feature || !feature.properties) { - console.error('Invalid feature properties'); - return ''; - } - if (!feature.properties.pm2_5 || !feature.properties.aqi) { - console.error('Invalid AQI or PM2.5 data'); - return ''; - } + try { + validateFeatureProperties(feature); + } catch (error) { + console.error(error.message); + return ''; + }
Line range hint
276-309: Extract date formatting logic to a utility functionThe date formatting logic could be reused across components. Consider extracting it to a shared utility function.
+const formatDate = (date) => { + return new Date(date).toLocaleDateString('en-US', { + year: 'numeric', + month: 'long', + day: '2-digit', + }); +}; return ` <div class="flex flex-col gap-2 p-3 bg-white rounded-lg shadow-lg" style="min-width: 250px; width: max-content;"> <div class="text-gray-500 text-xs font-normal font-sans leading-none"> - ${new Date(feature.properties.createdAt).toLocaleDateString('en-US', { - year: 'numeric', - month: 'long', - day: '2-digit', - })} + ${formatDate(feature.properties.createdAt)} </div>platform/src/common/components/Charts/components/index.jsx (3)
22-77: Consider relocating 'pollutantRanges' to a shared constants fileThe
pollutantRangesobject is quite comprehensive and might be useful across multiple components. Placing it in a shared constants or configuration file can improve modularity and make maintenance smoother.
79-117: Extract 'categoryDetails' for better reusabilitySimilarly, the
categoryDetailsobject could be beneficial in other parts of the application. Moving it to a shared location enhances reusability and keeps things tidy.
22-77: Simplify 'pollutantRanges' to minimize redundancyThe
pollutantRangesobject repeats similar category data for each pollutant. Considering a base category set and mapping pollutants to these ranges could streamline updates and reduce redundancy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
platform/src/common/components/Charts/components/index.jsx(3 hunks)platform/src/common/components/Map/components/MapNodes.js(1 hunks)
🔇 Additional comments (2)
platform/src/common/components/Map/components/MapNodes.js (1)
Line range hint 41-47: Verify PM2.5 thresholds against WHO guidelines
The PM2.5 thresholds appear to follow a standard scale, but let's verify these values against WHO Air Quality Guidelines to ensure compliance with international standards.
platform/src/common/components/Charts/components/index.jsx (1)
165-165: Double-check that 'pollutionType' is always set in Redux state
Just to be safe, ensure that pollutionType is always defined in the Redux state. If there's a chance it might be undefined, adding a default value or error handling can prevent any hiccups.
Run the following script to verify that pollutionType is initialized in all reducers:
✅ Verification successful
pollutionType is properly initialized and managed in Redux state
The verification shows that pollutionType is well-handled in the Redux store:
- It's initialized with a default value of 'pm2_5' in the chart reducer (ChartSlice.js)
- It has a dedicated setter action 'setPollutant'
- The value is consistently accessed through Redux selectors across components
- There's proper state management when changing pollutant types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'pollutionType' is initialized in Redux initial states.
# Test: Search for initial state definitions containing 'pollutionType'.
# Expect: All initial states should define 'pollutionType' with a default value.
rg "const initialState\s*=\s*{[^}]*pollutionType\s*:" -A 5
Length of output: 217
Script:
#!/bin/bash
# Let's try a different approach to find Redux state initialization
# First, let's find files that might contain Redux state
rg -l "initialState.*pollutionType|pollutionType.*initialState"
# Then look for how pollutionType is used in the component
rg "pollutionType" platform/src/common/components/Charts/components/index.jsx -C 3
# Also check for any Redux related imports and connections
rg "connect|useSelector|useDispatch" platform/src/common/components/Charts/components/index.jsx -C 2
Length of output: 1826
Script:
#!/bin/bash
# Let's check the chart reducer to see how pollutionType is managed
rg -l "chart.*reducer|reducer.*chart"
# Then examine the content of any chart reducer files
fd "chart.*reducer|reducer.*chart" --exec cat {}
# Also check for any actions that might set pollutionType
rg "SET.*POLLUTION.*TYPE|pollutionType" --type js -C 2
Length of output: 7784
| export const getAirQualityLevelText = (value, pollutionType) => { | ||
| const ranges = pollutantRanges[pollutionType] || []; | ||
|
|
||
| // Find the appropriate category based on the value | ||
| const category = | ||
| ranges.find((range) => value >= range.limit)?.category || 'Invalid'; | ||
|
|
||
| return { airQualityText, AirQualityIcon, airQualityColor }; | ||
| // Retrieve the details for the category | ||
| const { text, icon, color } = categoryDetails[category] || {}; | ||
|
|
||
| return { | ||
| airQualityText: text || 'Unknown Air Quality', | ||
| AirQualityIcon: icon, | ||
| airQualityColor: color || 'text-gray-300', | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle unexpected 'pollutionType' values in 'getAirQualityLevelText'
Currently, if an invalid pollutionType is provided, the function defaults to 'Invalid Air Quality Data'. It might be helpful to add validation or default cases to gracefully manage unexpected values.
There was a problem hiding this comment.
Hi @OchiengPaul442 , this is impressive. I have left a few comments for your review and consideration.
| @@ -19,44 +19,123 @@ export const truncate = (str) => { | |||
| return str.length > 20 ? str.substr(0, 20 - 1) + '...' : str; | |||
| }; | |||
|
|
|||
| const pollutantRanges = { | |||
| pm2_5: [ | |||
There was a problem hiding this comment.
Hi @OchiengPaul442 , why are we still "hard coding" here?
Can we try to avoid hard coding since this will definitely create more work for us in the near future?
| @@ -7,7 +7,7 @@ export default async function handler(req, res) { | |||
|
|
|||
| // Set up the target URL | |||
| const targetUrl = | |||
| 'https://platform.airqo.net/api/v2/analytics/dashboard/chart/d3/data?tenant=airqo'; | |||
| 'https://analytics.airqo.net/api/v2/analytics/dashboard/chart/d3/data?tenant=airqo'; | |||
There was a problem hiding this comment.
Hi @OchiengPaul442 , can't this be an environmental variable?
| }, | ||
| { max: 500, text: 'Air Quality is Hazardous', status: 'hazardous' }, | ||
| ]; | ||
| // Mapping from aqi_category to IconMap keys |
There was a problem hiding this comment.
Would this mapping be necessary if we just used the single word color_names? That is:
- Green
- Yellow
- Orange
- Red
- Purple
- Maroon
And then you use those specific color_names to map to whatever you want in the code below. As you do this mapping, you will NOT be using the colors but rather, you will be using their names as KEYS (identifiers) of the different categories.
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
Release Notes
New Features
Improvements
Bug Fixes
These changes enhance the overall performance and reliability of the application.