Analytics: Toggling data for different locations #2335
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several enhancements across various components, primarily focusing on the integration of the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
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.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (17)
platform/src/core/hooks/useUserPreferences.jsx (3)
13-48: RefactorisMountedpattern to prevent potential memory leaksThe current use of the
isMountedflag within theuseEffecthook may not reliably prevent state updates on unmounted components due to how closures work in functional components. Instead, consider using theuseRefhook to track the mounted status or cancel the asynchronous operation in the cleanup function to avoid potential memory leaks.Here's how you can update the code using
useRef:import { useEffect, useRef } from 'react'; const useUserPreferences = () => { const dispatch = useDispatch(); const { id: activeGroupId, userID } = useGetActiveGroup(); + const isMountedRef = useRef(true); useEffect(() => { - let isMounted = true; const fetchPreferences = async () => { if (!userID) { console.warn('No user ID available to fetch preferences.'); return; } try { // Dispatch the action to fetch user preferences await dispatch( getIndividualUserPreferences({ identifier: userID, groupID: activeGroupId, }), ); // Only proceed if the component is still mounted - if (isMounted) { + if (isMountedRef.current) { console.log('User preferences fetched successfully.'); } } catch (error) { if (isMountedRef.current) { console.error('Error fetching user preferences:', error); } } }; fetchPreferences(); // Cleanup function to set the isMounted flag to false upon unmounting return () => { - isMounted = false; + isMountedRef.current = false; }; }, [activeGroupId, userID]); };
49-49: Removedispatchfrom the dependency arrayThe
dispatchfunction returned byuseDispatch()is stable and does not change between renders, so it's safe to omit it from the dependency array ofuseEffect.Update the dependency array as follows:
- }, [dispatch, activeGroupId, userID]); + }, [activeGroupId, userID]);
20-20: Replace console statements with appropriate logging or remove themUsing
console.warn,console.log, andconsole.errorin production code can clutter logs and may not be suitable for production environments. Consider using a dedicated logging library or remove these statements if they are not necessary.Also applies to: 34-34, 38-38
platform/src/core/hooks/useFetchAnalyticsData.jsx (1)
Line range hint
142-153: Enhance error message with organization contextConsider including the organization name in the error message to help users identify which organization's data failed to load.
setState((prev) => ({ ...prev, error: new ApiError( - error.message || 'Failed to fetch data', + error.message || `Failed to fetch data for organization: ${organisationName}`, error.status, error, ), loading: false, }));platform/src/pages/account/login/index.jsx (2)
74-74: Consider enhancing error handling for preferences fetchWhile the change to object parameter is good, consider adding specific error handling for the preferences fetch failure.
-dispatch(getIndividualUserPreferences({ identifier: user._id })), +try { + await dispatch(getIndividualUserPreferences({ identifier: user._id })).unwrap(); +} catch (error) { + console.error('Failed to fetch user preferences:', error); + // Continue with default preferences +}
Line range hint
19-20: Consider adding jitter to retry mechanismThe retry mechanism is good, but adding jitter would help prevent thundering herd problems in case of multiple simultaneous retries.
const MAX_RETRIES = 3; -const RETRY_DELAY = 1000; +const BASE_DELAY = 1000; const retryWithDelay = async (fn, retries = MAX_RETRIES) => { try { return await fn(); } catch (error) { if (retries > 0 && error.response?.status === 429) { - await new Promise((resolve) => setTimeout(resolve, RETRY_DELAY)); + const jitter = Math.random() * 0.3 * BASE_DELAY; + const delay = BASE_DELAY * (MAX_RETRIES - retries + 1) + jitter; + await new Promise((resolve) => setTimeout(resolve, delay)); return retryWithDelay(fn, retries - 1); } throw error; } };Also applies to: 39-49
platform/src/lib/store/services/charts/ChartSlice.js (1)
211-224: Consider extracting the duplicated chartSites handling logic.The chartSites validation and comparison logic is duplicated between
setChartSitesandsetChartDataAtOncereducers.Consider extracting the logic into a helper function:
+const updateChartSites = (state, newSites) => { + if (!Array.isArray(newSites) || newSites.length === 0) { + if (state.chartSites.length !== 0) { + state.chartSites = []; + } + } else { + const isDifferent = + state.chartSites.length !== newSites.length || + !newSites.every((site, index) => site === state.chartSites[index]); + + if (isDifferent) { + state.chartSites = newSites; + } + } +}; setChartSites: (state, action) => { - // Current implementation + updateChartSites(state, action.payload); }, setChartDataAtOnce: (state, action) => { const allowedKeys = Object.keys(initialState); Object.entries(action.payload).forEach(([key, value]) => { if (allowedKeys.includes(key)) { if (key === 'chartSites') { - // Current implementation + updateChartSites(state, value); } else { if (state[key] !== value) { state[key] = value; } } } }); },platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx (2)
Line range hint
167-181: Enhance error handling in preference updatesThe group context integration looks good, but the error handling could be more robust.
Consider this improvement:
const payload = { user_id: userID, group_id: activeGroupId, selected_sites: selectedSitesData, }; dispatch(replaceUserPreferences(payload)) .then(() => { onClose(); - if (userID) { + if (userID && activeGroupId) { dispatch( getIndividualUserPreferences({ identifier: userID, groupID: activeGroupId, }), ); + } else { + console.warn('Missing userID or activeGroupId for preference update'); } dispatch(setRefreshChart(true)); })
Line range hint
141-145: Consider improving the location limit UXWhile the validation works, consider disabling selection once the limit is reached rather than showing an error after the fact.
This would provide better user feedback and prevent invalid selections upfront.
platform/src/common/components/AQNumberCard/index.jsx (2)
48-50: Enhance trend message clarityWhile the messages are more user-friendly, consider adding the specific AQI values for better context.
- trendTooltip = `The Air quality has increased by ${percentageDifference}% compared to the previous week, indicating deteriorating air quality.`; + trendTooltip = `The Air quality has increased from ${averages.previousValue.toFixed(1)} to ${averages.currentValue.toFixed(1)} (${percentageDifference}% change), indicating deteriorating air quality.`; - trendTooltip = `The Air quality has decreased by ${percentageDifference}% compared to the previous week, indicating improving air quality.`; + trendTooltip = `The Air quality has decreased from ${averages.previousValue.toFixed(1)} to ${averages.currentValue.toFixed(1)} (${percentageDifference}% change), indicating improving air quality.`;
274-275: Optimize data refresh logicAdding activeGroupId to dependencies could cause unnecessary refreshes. Consider using a ref to track group changes.
+ const previousGroupRef = useRef(activeGroupId); + + useEffect(() => { + if (previousGroupRef.current !== activeGroupId) { + previousGroupRef.current = activeGroupId; + // Your existing fetch logic here + } + }, [activeGroupId]);Also applies to: 319-319
platform/src/common/components/Charts/MoreInsightsChart.jsx (3)
33-46: Great documentation improvements!The added JSDoc comments significantly improve code readability. Consider adding @throws tags where applicable.
Also applies to: 66-69, 119-122, 176-178, 196-198
Line range hint
217-244: Align empty state styling with design systemThe empty state handling is good, but consider using consistent styling classes.
- <div className="w-full flex flex-col justify-center items-center h-full text-gray-500 p-4"> + <div className="w-full flex flex-col justify-center items-center h-full text-gray-500 p-4 empty-state"> - <p className="text-lg font-medium mb-2">No Data Available</p> + <p className="text-lg font-semibold mb-2 empty-state-title">No Data Available</p>
125-133: Consider TypeScript migration for better type safetyThe type checking is good, but TypeScript would provide compile-time safety and better IDE support.
This would help prevent runtime errors and improve maintainability.
platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx (3)
80-82: Consider memoizing the activeGroup objectSince activeGroup is derived from props (activeGroupId and groupTitle), consider using useMemo to prevent unnecessary recreations.
- const activeGroup = { id: activeGroupId, name: groupTitle }; + const activeGroup = useMemo( + () => ({ id: activeGroupId, name: groupTitle }), + [activeGroupId, groupTitle] + );
100-101: Add defensive null checks for organization defaultsThe current implementation might be vulnerable to null/undefined values. Consider adding more robust null checks.
- organization: activeGroup || ORGANIZATION_OPTIONS[0], + organization: activeGroup?.id ? activeGroup : ORGANIZATION_OPTIONS?.[0] ?? null,Similarly for the setFormData call:
- organization: activeGroup || airqoNetwork, + organization: (activeGroup?.id ? activeGroup : airqoNetwork) ?? null,Also applies to: 122-124
126-126: Enhance effect condition specificityThe effect's dependency array is correct, but consider making the condition more specific to avoid unnecessary updates.
useEffect(() => { - if (ORGANIZATION_OPTIONS.length > 0 && !formData.organization) { + if ( + ORGANIZATION_OPTIONS.length > 0 && + (!formData.organization || formData.organization.id !== activeGroupId) + ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
platform/src/common/components/AQNumberCard/index.jsx(4 hunks)platform/src/common/components/Charts/ChartContainer.jsx(1 hunks)platform/src/common/components/Charts/MoreInsightsChart.jsx(10 hunks)platform/src/common/components/Layout/index.jsx(2 hunks)platform/src/common/components/Modal/dataDownload/components/AirQualityCard.jsx(3 hunks)platform/src/common/components/Modal/dataDownload/components/TableLoadingSkeleton.jsx(0 hunks)platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx(5 hunks)platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx(7 hunks)platform/src/core/apis/Account.js(1 hunks)platform/src/core/hooks/useFetchAnalyticsData.jsx(1 hunks)platform/src/core/hooks/useGetActiveGroupId.jsx(1 hunks)platform/src/core/hooks/useUserPreferences.jsx(1 hunks)platform/src/lib/store/services/account/UserDefaultsSlice.js(1 hunks)platform/src/lib/store/services/charts/ChartSlice.js(5 hunks)platform/src/pages/account/login/index.jsx(3 hunks)platform/src/pages/analytics/_components/OverView.jsx(2 hunks)
💤 Files with no reviewable changes (1)
- platform/src/common/components/Modal/dataDownload/components/TableLoadingSkeleton.jsx
✅ Files skipped from review due to trivial changes (1)
- platform/src/common/components/Modal/dataDownload/components/AirQualityCard.jsx
🔇 Additional comments (9)
platform/src/common/components/Layout/index.jsx (1)
32-37: Integration of useGetActiveGroup looks good
The use of useGetActiveGroup to retrieve userID and passing it to useInactivityLogout streamlines the user identification process and ensures consistency across components.
platform/src/lib/store/services/account/UserDefaultsSlice.js (1)
50-52: Updated function signature enhances flexibility
Changing the getIndividualUserPreferences function to accept an object with identifier and groupID improves clarity and allows for optional group context when fetching user preferences.
platform/src/core/hooks/useFetchAnalyticsData.jsx (1)
96-96: Consider backward compatibility impact of removing default value
The removal of the default value for organisationName makes it a required parameter, which could break existing implementations that rely on the default 'airqo' value.
Let's verify the usage of this hook across the codebase:
platform/src/core/apis/Account.js (1)
167-178: Well-structured API enhancement with proper error handling
The changes look good with:
- Proper optional parameter handling for groupID
- Robust error handling with fallback message
- Clean implementation of query parameters
platform/src/pages/account/login/index.jsx (1)
120-120: LGTM: Proper use of HTML entities in JSX
Good practice using HTML entities for apostrophes in JSX text.
Also applies to: 189-189
platform/src/pages/analytics/_components/OverView.jsx (1)
24-24: LGTM! Good use of the useGetActiveGroup hook.
The changes effectively integrate the active group's title as a fallback for the organization name, improving the component's resilience to missing data.
Also applies to: 30-30, 50-50
platform/src/lib/store/services/charts/ChartSlice.js (1)
4-5: Great job on adding comprehensive JSDoc comments!
The addition of parameter documentation significantly improves code maintainability and developer experience.
Also applies to: 49-50, 62-63, 75-76, 89-90, 105-107, 132-133, 145-146, 158-159, 170-172, 181-182, 194-194, 204-205
platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx (1)
11-11: Verify the useGetActiveGroup hook implementation
The integration of useGetActiveGroup hook improves group context management, but we should ensure it handles edge cases properly.
Also applies to: 47-48, 71-74
✅ Verification successful
Let me gather more information about the hook's implementation and error handling.
The first pattern will show us the complete hook implementation, and the second set of commands will help us identify any error handling patterns around the hook's return values.
The useGetActiveGroup hook implementation is robust and safe to use
The hook has proper error handling and fallback mechanisms:
- Handles missing userInfo or groups with null returns
- Uses optional chaining for safe state access
- Provides fallback to localStorage when no matching group is found
- Returns consistent object structure in all cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the useGetActiveGroup hook implementation
# Test: Check if the hook handles undefined/null cases
# Search for error handling in the hook implementation
ast-grep --pattern 'export function useGetActiveGroup() {
$$$
return { id: $_, title: $_ }
}'
# Check for usage patterns and error handling
rg -A 3 "useGetActiveGroup"
Length of output: 4960
Script:
#!/bin/bash
# Check the complete implementation of useGetActiveGroup
ast-grep --pattern 'export function useGetActiveGroup() {
$$$
}'
# Look for error handling patterns in files using this hook
rg -B 2 -A 2 "(!|undefined|null).*activeGroupId"
rg -B 2 -A 2 "(!|undefined|null).*groupTitle"
Length of output: 2819
platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx (1)
25-25: Clean hook integration!
The useGetActiveGroup hook is properly imported and destructured with meaningful variable names.
Also applies to: 57-61
| const chartData = useSelector((state) => state.chart); | ||
|
|
||
| // get activeGroup from local storage | ||
| const activeGroup = JSON.parse(localStorage.getItem('activeGroup')); |
There was a problem hiding this comment.
Handle potential null value when parsing activeGroup from local storage
If localStorage.getItem('activeGroup') returns null, parsing it with JSON.parse will result in activeGroup being null. Later attempts to access properties like activeGroup._id would cause a runtime error. It's important to check if the parsed activeGroup is not null before using it.
You can update the code to handle this case:
const activeGroupData = localStorage.getItem('activeGroup');
- const activeGroup = JSON.parse(localStorage.getItem('activeGroup'));
+ const activeGroup = activeGroupData ? JSON.parse(activeGroupData) : null;📝 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 activeGroup = JSON.parse(localStorage.getItem('activeGroup')); | |
| const activeGroupData = localStorage.getItem('activeGroup'); | |
| const activeGroup = activeGroupData ? JSON.parse(activeGroupData) : null; |
| id: activeGroup._id, | ||
| title: activeGroup.grp_title, | ||
| userID: userInfo._id, | ||
| groupList: userInfo.groups, | ||
| }; |
There was a problem hiding this comment.
Add null check for activeGroup before accessing its properties
When no matching group is found, and activeGroup is null, accessing activeGroup._id and activeGroup.grp_title will result in an error. Ensure that activeGroup is not null before accessing its properties to prevent runtime exceptions.
Modify the return statement to handle activeGroup being null:
return matchingGroup
? {
id: matchingGroup._id,
title: matchingGroup.grp_title,
userID: userInfo._id,
groupList: userInfo.groups,
}
- : {
+ : activeGroup
? {
id: activeGroup._id,
title: activeGroup.grp_title,
userID: userInfo._id,
groupList: userInfo.groups,
}
+ : {
+ id: null,
+ title: null,
+ userID: userInfo._id,
+ groupList: userInfo.groups,
+ };Committable suggestion skipped: line range outside the PR's diff.
| @@ -238,7 +238,7 @@ const ChartContainer = ({ | |||
| </div> | |||
| )} | |||
| <div ref={chartRef} className="my-3 relative" style={{ width, height }}> | |||
| {error ? ( | |||
| {error && !chartSites.length === 0 ? ( | |||
There was a problem hiding this comment.
Fix the logical error in the error overlay condition.
The current condition error && !chartSites.length === 0 is incorrect. It's equivalent to showing the error when there's an error AND there are chart sites, which seems opposite to the intent.
Apply this fix:
- {error && !chartSites.length === 0 ? (
+ {error && chartSites.length === 0 ? (This will show the error overlay when there's an error AND there are no chart sites selected.
📝 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.
| {error && !chartSites.length === 0 ? ( | |
| {error && chartSites.length === 0 ? ( |
| () => | ||
| userInfo?.groups?.map((group) => ({ | ||
| groupList?.map((group) => ({ | ||
| id: group._id, | ||
| name: group.grp_title, | ||
| })) || [], |
There was a problem hiding this comment.
Fix dependency array in ORGANIZATION_OPTIONS useMemo
The useMemo dependency array references userInfo but the calculation uses groupList. This mismatch could lead to stale data.
const ORGANIZATION_OPTIONS = useMemo(
() =>
groupList?.map((group) => ({
id: group._id,
name: group.grp_title,
})) || [],
- [userInfo],
+ [groupList],
);📝 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.
| () => | |
| userInfo?.groups?.map((group) => ({ | |
| groupList?.map((group) => ({ | |
| id: group._id, | |
| name: group.grp_title, | |
| })) || [], | |
| () => | |
| groupList?.map((group) => ({ | |
| id: group._id, | |
| name: group.grp_title, | |
| })) || [], | |
| [groupList], |
|
New next-platform changes available for preview here |
|
New next-platform changes available for preview here |
Summary of Changes (What does this PR do?)
Summary by CodeRabbit
Release Notes
New Features
useGetActiveGrouphook.Bug Fixes
Documentation
Chores