Analytics: Updates to display locations by groups #2311
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several enhancements across multiple components related to site summary fetching and data handling. Key changes include the addition of a Changes
Possibly related PRs
Suggested labels
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 (
|
|
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: 2
🧹 Outside diff range and nitpick comments (4)
platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx (3)
103-116: Refactor duplicated logic for selecting the default networkThe logic for determining the default network, prioritizing "airqo" if available, is implemented both in the
useEffecthook (lines 107-114) and thehandleClearSelectionfunction (lines 134-140). To enhance maintainability and reduce code duplication, consider extracting this logic into a separate helper function.Suggested refactor:
+// Helper function to get the default network +const getDefaultNetwork = () => { + const airqoNetwork = NETWORK_OPTIONS.find( + (network) => network.name.toLowerCase() === 'airqo', + ); + return airqoNetwork || NETWORK_OPTIONS[0] || { id: '', name: 'Default Network' }; +}; ... // In useEffect hook useEffect(() => { if (NETWORK_OPTIONS.length > 0 && !formData.network) { setFormData((prevData) => ({ ...prevData, - network: airqoNetwork || NETWORK_OPTIONS[0], + network: getDefaultNetwork(), })); } }, [NETWORK_OPTIONS, formData.network]); ... // In handleClearSelection function const handleClearSelection = useCallback(() => { ... setFormData({ title: { name: 'Untitled Report' }, - network: airqoNetwork || - NETWORK_OPTIONS[0] || { id: '', name: 'Default Network' }, + network: getDefaultNetwork(), ... }); ... }, [NETWORK_OPTIONS]);Also applies to: 134-140
215-217: Use accurate date calculations for duration validationThe current validation uses fixed month durations (30 days), which could lead to inaccuracies due to varying month lengths. To improve accuracy, consider using the
differenceInMonthsfunction fromdate-fns.Suggested change:
-import { format } from 'date-fns'; +import { format, differenceInMonths } from 'date-fns'; ... const validateDuration = (frequency, startDate, endDate) => { - const sixMonthsInMs = 6 * 30 * 24 * 60 * 60 * 1000; // 6 months in milliseconds - const oneYearInMs = 12 * 30 * 24 * 60 * 60 * 1000; // 1 year in milliseconds - const durationMs = endDate - startDate; + const durationMonths = differenceInMonths(endDate, startDate); - if (frequency === 'hourly' && durationMs > sixMonthsInMs) { + if (frequency === 'hourly' && durationMonths > 6) { return 'For hourly frequency, the duration cannot exceed 6 months.'; } - if (frequency === 'daily' && durationMs > oneYearInMs) { + if (frequency === 'daily' && durationMonths > 12) { return 'For daily frequency, the duration cannot exceed 1 year.'; } return null; };This adjustment accounts for the actual number of days in each month, providing a more precise validation.
299-302: Enhance user feedback in error handlingWhen setting the form error (lines 299-302), consider providing user-friendly error messages that guide the user on how to resolve the issue. Tailoring error messages based on specific exceptions can significantly improve the user experience.
platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx (1)
65-72: Consider enhancing the useEffect implementation.While the basic functionality is there, consider these improvements for better error handling and performance:
- Add a guard clause for undefined organizationName
- Handle loading state
- Add error handling within the effect
Here's a suggested implementation:
useEffect(() => { - if (chartData.organizationName) { + if (!chartData.organizationName) return; + + const fetchData = async () => { + try { + setLoading(true); dispatch(fetchSitesSummary({ group: chartData.organizationName })); + } catch (error) { + console.error('Failed to fetch sites:', error); + } finally { + setLoading(false); + } } + + fetchData(); }, [dispatch, chartData.organizationName]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx(3 hunks)platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx(10 hunks)platform/src/core/apis/Analytics.js(1 hunks)platform/src/lib/store/services/sitesSummarySlice/index.js(3 hunks)platform/src/pages/analytics/index.jsx(0 hunks)platform/src/pages/api/proxy/sites.js(1 hunks)
💤 Files with no reviewable changes (1)
- platform/src/pages/analytics/index.jsx
🔇 Additional comments (4)
platform/src/core/apis/Analytics.js (1)
25-34: Inclusion of 'group' parameter enhances API flexibility
Adding the group parameter to the getSitesSummaryApi function allows for more specific data retrieval based on user group selection. The implementation correctly integrates the parameter into the Axios request.
platform/src/lib/store/services/sitesSummarySlice/index.js (1)
8-21: Integration of 'group' parameter improves data specificity
By introducing the group parameter in the fetchSitesSummary thunk, the application can now fetch site summaries tailored to specific user groups. This change enhances data relevance and aligns with user selections.
Also applies to: 31-31
platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx (2)
10-10: LGTM! Clean integration of site summary fetching.
The addition of fetchSitesSummary and chartData selector aligns well with the PR's objective of enabling group-based location fetching.
Also applies to: 38-38
Line range hint 1-266: Verify group-based filtering and consider architectural improvements.
While the component works as intended, here are some architectural considerations:
- Consider extracting the site fetching and filtering logic into a custom hook for better reusability and testing
- The filteredSites memo could be optimized to include group-based filtering
Let's verify the group-based filtering implementation:
✅ Verification successful
Group-based filtering and online status filtering are properly implemented.
The verification shows that:
- Sites are correctly filtered by organization through
fetchSitesSummary({ group: chartData.organizationName })in the Redux slice - Online status filtering is implemented using
site.isOnline === truein a memoized function - The filtering chain is properly connected:
- Organization selection triggers API fetch with group parameter
- Results are filtered for online sites
- UI components receive and display filtered data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of group-based filtering across the codebase
# Check for any existing group-based filtering logic
echo "Checking for group-based filtering implementations..."
rg -l "group.*filter|filter.*group" --type js --type jsx
# Verify the API integration
echo "Verifying API integration..."
ast-grep --pattern 'fetchSitesSummary({ $$ })'
# Check for potential edge cases in filtering
echo "Checking for edge case handling..."
rg "isOnline.*true" --type js --type jsx -C 2
Length of output: 474
Script:
#!/bin/bash
# Let's modify our approach to handle the file extensions correctly
# Check for group-based filtering implementations
echo "Checking for group-based filtering implementations..."
rg -t js "group.*filter|filter.*group"
# Verify the API integration for fetchSitesSummary
echo "Verifying API integration..."
rg "fetchSitesSummary" -t js -C 2
# Check for online status filtering implementation
echo "Checking online status filtering..."
rg "isOnline" -t js -C 2
# Check the sites summary slice implementation
echo "Checking sites summary slice..."
fd -e js -e jsx sitesSummarySlice
Length of output: 66460
Script:
#!/bin/bash
# Let's try a more focused approach to verify the implementation
# Check the sites summary slice implementation
echo "Checking sites summary slice implementation..."
fd sitesSummarySlice -t js -t jsx -x cat {}
# Check fetchSitesSummary usage
echo "Checking fetchSitesSummary usage..."
rg "fetchSitesSummary" -t js -t jsx -A 2 -B 2
# Check online status filtering
echo "Checking online status filtering..."
rg "isOnline.*true" -t js -t jsx -A 2 -B 2
# Check organization-based filtering
echo "Checking organization-based filtering..."
rg "organizationName.*group" -t js -t jsx -A 2 -B 2
Length of output: 802
Script:
#!/bin/bash
# Let's try without file type restrictions since the extensions might be mixed
# Check the sites summary slice implementation
echo "Checking sites summary slice implementation..."
fd sitesSummarySlice -x cat {}
# Check fetchSitesSummary usage
echo "Checking fetchSitesSummary usage..."
rg "fetchSitesSummary" -A 2 -B 2
# Check online status filtering
echo "Checking online status filtering..."
rg "isOnline.*true" -A 2 -B 2
# Check organization-based filtering
echo "Checking organization-based filtering..."
rg "chartData\.organizationName" -A 2 -B 2
Length of output: 10156
| // Handle errors and respond appropriately | ||
| res.status(error.response?.status || 500).json({ | ||
| error: 'Error while proxying request', | ||
| message: error.response?.data?.message || error.message, |
There was a problem hiding this comment.
Prevent exposure of internal error details to the client
Returning error.response?.data?.message or error.message directly to the client may inadvertently reveal sensitive internal information. It's prudent to send a generic error message to the client while logging detailed errors on the server for debugging.
Suggested change:
res.status(error.response?.status || 500).json({
error: 'Error while proxying request',
- message: error.response?.data?.message || error.message,
+ message: 'An unexpected error occurred. Please try again later.',
});Also, log the detailed error server-side:
console.error('Proxy error:', error);| // Pass query parameters from the request to the target URL | ||
| const response = await axios.get(targetUrl, { | ||
| headers: { | ||
| Authorization: req.headers.authorization, | ||
| Authorization: req.headers.authorization, // Pass the authorization header | ||
| }, | ||
| params: req.query, // Include query parameters from the request |
There was a problem hiding this comment.
Validate query parameters before forwarding
Passing req.query directly as params (line 17) without validation might introduce security risks, such as injection attacks. It's advisable to validate or sanitize query parameters to ensure they meet expected formats and values before forwarding them.
Suggested approach:
- Define allowed query parameters and their expected formats.
- Use a validation library like
joior manually check each parameter.
Example:
const allowedParams = ['group'];
const params = {};
allowedParams.forEach((param) => {
if (req.query[param]) {
params[param] = req.query[param];
}
});
// Then pass `params` instead of `req.query`
...
params,
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx (2)
38-38: Consider destructuring specific chart properties.Instead of selecting the entire chart state, consider destructuring only the required
organizationNameproperty to minimize unnecessary re-renders.-const chartData = useSelector((state) => state.chart); +const { organizationName } = useSelector((state) => state.chart);
Line range hint
38-72: Consider architectural improvements for better maintainability.The component handles multiple responsibilities and complex state management. Consider these architectural improvements:
- Extract site selection logic into a custom hook (e.g.,
useSiteSelection)- Create a separate hook for data fetching logic (e.g.,
useSitesFetching)- Consider adding TypeScript for better type safety
- Add unit tests for the complex selection and submission logic
Example custom hook structure:
const useSitesFetching = (organizationName) => { const dispatch = useDispatch(); const [loading, setLoading] = useState(false); const [error, setError] = useState(null); useEffect(() => { // ... fetching logic }, [organizationName, dispatch]); return { loading, error }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx(3 hunks)platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx(10 hunks)platform/src/core/apis/Analytics.js(1 hunks)platform/src/lib/store/services/sitesSummarySlice/index.js(3 hunks)platform/src/pages/analytics/index.jsx(0 hunks)platform/src/pages/api/proxy/sites.js(1 hunks)
💤 Files with no reviewable changes (1)
- platform/src/pages/analytics/index.jsx
🔇 Additional comments (3)
platform/src/core/apis/Analytics.js (1)
25-34: Inclusion of 'group' Parameter Enhances Functionality
The addition of the group parameter and its incorporation into the API call appropriately extends the function's capability to fetch data based on group selection.
platform/src/lib/store/services/sitesSummarySlice/index.js (1)
3-8: Async Thunk Updated Correctly to Accept 'group' Parameter
The modifications to fetchSitesSummary to include the group parameter are well implemented, allowing for more dynamic data fetching based on group context.
platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx (1)
10-10: LGTM! Clean import addition.
The import follows the established pattern and uses the alias path convention correctly.
| setFormError( | ||
| error.message || | ||
| 'An error occurred while downloading. Please try again.', | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Displaying Technical Error Messages to Users
In setFormError, using error.message directly can expose technical details to the user. It's better to display a generic error message to enhance user experience and security.
Apply this diff to implement the change:
-setFormError(
- error.message ||
- 'An error occurred while downloading. Please try again.',
-);
+setFormError('An error occurred while downloading. Please try again.');📝 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.
| setFormError( | |
| error.message || | |
| 'An error occurred while downloading. Please try again.', | |
| ); | |
| setFormError('An error occurred while downloading. Please try again.'); |
| // Handle errors and respond appropriately | ||
| res.status(error.response?.status || 500).json({ | ||
| error: 'Error while proxying request', | ||
| message: error.response?.data?.message || error.message, |
There was a problem hiding this comment.
Prevent Exposing Internal Error Details in API Responses
Currently, the error response may include internal error messages from error.response.data.message or error.message, which could reveal sensitive information. Consider returning a generic error message to the client and logging the detailed error server-side.
Apply this diff to address the issue:
- res.status(error.response?.status || 500).json({
- error: 'Error while proxying request',
- message: error.response?.data?.message || error.message,
- });
+ console.error('Error while proxying request:', error);
+ res.status(error.response?.status || 500).json({
+ error: 'Error while proxying request',
+ message: 'An error occurred while processing your request.',
+ });📝 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.
| // Handle errors and respond appropriately | |
| res.status(error.response?.status || 500).json({ | |
| error: 'Error while proxying request', | |
| message: error.response?.data?.message || error.message, | |
| // Handle errors and respond appropriately | |
| console.error('Error while proxying request:', error); | |
| res.status(error.response?.status || 500).json({ | |
| error: 'Error while proxying request', | |
| message: 'An error occurred while processing your request.', |
| /** | ||
| * Fetch sites summary whenever the selected organization changes. | ||
| */ | ||
| useEffect(() => { | ||
| if (chartData.organizationName) { | ||
| dispatch(fetchSitesSummary({ group: chartData.organizationName })); | ||
| } | ||
| }, [dispatch, chartData.organizationName]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the fetch implementation.
While the basic functionality is correct, consider these improvements:
- Add loading state management to prevent multiple concurrent requests
- Implement error handling for failed fetches
- Consider debouncing the fetch operation if organization changes are frequent
useEffect(() => {
+ const controller = new AbortController();
+
if (chartData.organizationName) {
- dispatch(fetchSitesSummary({ group: chartData.organizationName }));
+ dispatch(fetchSitesSummary({
+ group: chartData.organizationName,
+ signal: controller.signal
+ })).catch(error => {
+ if (!error.name === 'AbortError') {
+ console.error('Failed to fetch sites:', error);
+ }
+ });
}
+
+ return () => controller.abort();
}, [dispatch, chartData.organizationName]);Committable suggestion skipped: line range outside the PR's diff.
|
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/AQNumberCard/index.jsx (3)
30-60: Nice improvements to the trend data generation! 👍The renaming to
generateTrendDataand the enhanced tooltip messages make the code more maintainable and user-friendly. The JSDoc documentation is clear and helpful.Consider adding units to the tooltip messages for better clarity:
- trendTooltip = `AQI has increased by ${percentageDifference}% compared to the previous week, indicating deteriorating air quality.`; + trendTooltip = `Air Quality Index (AQI) has increased by ${percentageDifference}% compared to the previous week, indicating deteriorating air quality.`;
155-159: Good tooltip enhancements! 💡The placement and width control improvements enhance the user experience.
Consider making the tooltip width responsive:
- className="w-52" + className="w-52 lg:w-64"
253-256: Nice UX improvements to the Add Location button! ✨The enhanced styling and accessibility attributes make the button more user-friendly and accessible.
Consider adding a color transition to make the hover effect smoother:
- className="border-dashed border-2 border-blue-400 bg-blue-50 rounded-xl px-4 py-6 h-[220px] flex justify-center items-center text-blue-500 transition-transform transform hover:scale-95 focus:outline-none focus:ring-2 focus:ring-blue-500" + className="border-dashed border-2 border-blue-400 bg-blue-50 rounded-xl px-4 py-6 h-[220px] flex justify-center items-center text-blue-500 transition-all duration-200 transform hover:scale-95 hover:bg-blue-100 focus:outline-none focus:ring-2 focus:ring-blue-500"platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx (2)
80-87: Add error handling for user groups mapping.The current implementation assumes
userInfo.groupsis always valid. Consider adding null checks and providing a default empty array.const ORGANIZATION_OPTIONS = useMemo( () => - userInfo?.groups?.map((group) => ({ + (userInfo?.groups || []).map((group) => ({ id: group._id, name: group.grp_title, })) || [], [userInfo], );
106-116: Optimize organization initialization.The current implementation checks
ORGANIZATION_OPTIONSon every render. Consider moving the airqo network finding logic into the useMemo hook.+ const defaultOrganization = useMemo(() => { + if (!ORGANIZATION_OPTIONS.length) return null; + return ( + ORGANIZATION_OPTIONS.find( + (group) => group.name.toLowerCase() === 'airqo', + ) || ORGANIZATION_OPTIONS[0] + ); + }, [ORGANIZATION_OPTIONS]); useEffect(() => { - if (ORGANIZATION_OPTIONS.length > 0 && !formData.organization) { - const airqoNetwork = ORGANIZATION_OPTIONS.find( - (group) => group.name.toLowerCase() === 'airqo', - ); + if (defaultOrganization && !formData.organization) { setFormData((prevData) => ({ ...prevData, - organization: airqoNetwork || ORGANIZATION_OPTIONS[0], + organization: defaultOrganization, })); } - }, [ORGANIZATION_OPTIONS, formData.organization]); + }, [defaultOrganization, formData.organization]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
platform/src/common/components/AQNumberCard/index.jsx(9 hunks)platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx(11 hunks)platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx(2 hunks)platform/src/common/styles/global.scss(1 hunks)platform/src/pages/api/proxy/sites.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- platform/src/pages/api/proxy/sites.js
🔇 Additional comments (8)
platform/src/common/components/AQNumberCard/index.jsx (3)
68-81: Great accessibility improvements! 🎯
The enhanced aria-labels and tooltip placement provide better user experience and accessibility. The PropTypes update properly reflects the new data structure.
Also applies to: 106-106
144-144: LGTM: Consistent function name update
The function call has been correctly updated to match the renamed utility function.
220-223: Excellent accessibility consideration! ♿
Adding aria-hidden="true" to the decorative icon is a great accessibility practice, as it prevents screen readers from announcing non-essential content.
platform/src/common/styles/global.scss (1)
13-13: Consider alternative approaches to handle horizontal overflow.
While overflow-x: hidden on html, body prevents horizontal scrolling, it's a global solution that might mask underlying layout issues. Consider:
- Identifying specific components causing horizontal overflow
- Using container-level overflow control
- Implementing responsive design patterns
platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx (2)
164-171: LGTM! Clean implementation of site toggle functionality.
The use of useCallback and efficient state updates is well done.
299-302: Avoid exposing technical error messages to users.
platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (2)
364-364: LGTM! Clear and accurate tooltip content.
The tooltip content accurately reflects the data type being downloaded.
231-231: Verify if 'calibrated' is the only valid datatype value.
The hardcoded value 'calibrated' might need to be more flexible. Consider making it configurable or validating against allowed values.
✅ Verification successful
Let me search for more context about how this datatype is used.
Let me check how this dataType is being configured in the form data.
Let me search for the DATA_TYPE_OPTIONS constant which seems to define the valid values.
'calibrated' and 'raw' are the only valid datatype values
Based on the codebase analysis:
- DATA_TYPE_OPTIONS defines two options: 'Calibrated Data' and 'Raw Data'
- In DataDownload.jsx, these are converted to lowercase values 'calibrated' and 'raw'
- This is consistent with the dataTypeOptions in ExportData/index.js which also defines only these two values
The hardcoded value 'calibrated' is valid and there's no need for additional configuration as the system already handles both possible values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other datatype values in the codebase
# Search for datatype assignments
rg "datatype:\s*['\"](\w+)['\"]" --type js
Length of output: 155
Script:
#!/bin/bash
# Search for any references to datatype in the codebase
rg "datatype" -B 2 -A 2 --type js
# Search for any type validation or configuration related to datatype
ast-grep --pattern 'datatype'
Length of output: 1648
Script:
#!/bin/bash
# Search for dataType configuration in forms or initial data
rg "dataType" -B 3 -A 3 --type js
# Look for any enum or constant definitions related to data types
rg "const.*dataType|const.*DATA_TYPE" --type js
Length of output: 75604
Script:
#!/bin/bash
# Search for DATA_TYPE_OPTIONS definition
rg "DATA_TYPE_OPTIONS\s*=" -B 2 -A 2 --type js
# Also search for any other datatype related constants
rg "const.*[Dd]ata[Tt]ype.*=" -B 2 -A 2 --type js
Length of output: 66095
|
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):
Summary by CodeRabbit
New Features
Bug Fixes
Chores