analytics: update to data download and add location modules #2256
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
platform/src/core/urls/analytics.js (1)
10-10: Consider adding JSDoc documentation.Since this is a shared URL constant, it would be helpful to add JSDoc documentation explaining its purpose and noting that filtering should be handled at the component level.
+/** + * URL for fetching sites summary. + * Note: Filtering (e.g., by online status) should be handled at the component level. + */ export const SITES_SUMMARY_URL = `${BASE_AUTH_URL}/devices/sites/summary`;platform/src/pages/api/proxy/sites.js (1)
9-9: Consider caching strategy for performance optimizationSince we're now fetching all sites instead of just online ones, the response payload might be larger. Consider implementing caching at the API proxy level to optimize performance.
Example implementation:
+import { cacheData, getCachedData } from '../../../utils/cache'; + +const CACHE_KEY = 'sites_summary'; +const CACHE_TTL = 5 * 60 * 1000; // 5 minutes + export default async function handler(req, res) { if (req.method !== 'GET') { return res.status(405).json({ error: 'Method Not Allowed' }); } + + const cachedData = getCachedData(CACHE_KEY); + if (cachedData) { + return res.status(200).json(cachedData); + } + const targetUrl = 'https://analytics.airqo.net/api/v2/devices/sites/summary'; try { const response = await axios.get(targetUrl, { headers: { Authorization: req.headers.authorization, }, }); + cacheData(CACHE_KEY, response.data, CACHE_TTL); res.status(response.status).json(response.data);platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx (2)
52-55: Consider adding null safety to the isOnline checkThe current implementation might filter out sites where
isOnlineis undefined but should be considered valid.Consider this more robust implementation:
const filteredSites = useMemo(() => { - return (sitesSummaryData || []).filter((site) => site.isOnline === true); + return (sitesSummaryData || []).filter((site) => site?.isOnline !== false); }, [sitesSummaryData]);
74-81: Optimize the useEffect dependencies and add cleanupThe effect might run unnecessarily when
selectedSiteIdschanges butfilteredSitesis still loading.Consider this optimization:
useEffect(() => { - if (filteredSites && selectedSiteIds.length) { + if (!filteredSites?.length) return; + + if (selectedSiteIds.length) { const initialSelectedSites = filteredSites.filter((site) => selectedSiteIds.includes(site._id), ); setSelectedSites(initialSelectedSites); setSidebarSites(initialSelectedSites); } + + return () => { + // Cleanup function to prevent memory leaks + setSelectedSites([]); + setSidebarSites([]); + }; }, [filteredSites, selectedSiteIds]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx(3 hunks)platform/src/core/urls/analytics.js(1 hunks)platform/src/pages/api/proxy/sites.js(1 hunks)
🔇 Additional comments (4)
platform/src/core/urls/analytics.js (1)
10-10: LGTM! Let's verify API compatibility.
The removal of the online_status query parameter makes the endpoint more generic and reusable, which is good. This aligns well with moving the filtering logic to the component level.
Let's verify the API handler changes mentioned in the summary:
✅ Verification successful
API handler is compatible with the URL change
The API handler in platform/src/pages/api/proxy/sites.js is already designed to pass through requests without processing query parameters, making it fully compatible with the removal of online_status parameter. The handler simply forwards the request to the analytics endpoint with authentication headers, which is exactly what we want.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the API handler has been updated to handle the URL without the online_status parameter
# Expected: Find the corresponding handler file and check for related changes
# Search for the API handler file
rg -l "sites/summary" "platform/src/pages/api/proxy"
Length of output: 88
Script:
#!/bin/bash
# Let's examine the handler file to see how it processes the sites/summary endpoint
rg -A 10 "sites/summary" "platform/src/pages/api/proxy/sites.js"
# Also check for any online_status related code
rg "online_status" "platform/src/pages/api/proxy/sites.js"
Length of output: 427
platform/src/pages/api/proxy/sites.js (1)
9-9: Architectural change: Filter responsibility shifted to client
The removal of the online_status filter from the API endpoint aligns with the PR objective of making online status handling specific to the AddLocations component. This is a good architectural decision as it:
- Provides more flexibility in how different components can filter the data
- Reduces the coupling between the API and specific UI requirements
However, we should verify the impact of this change.
Let's verify the usage of this endpoint across the codebase:
✅ Verification successful
Removal of online_status filter is safe to proceed
The codebase analysis reveals that the removal of the online_status filter from the sites summary endpoint is architecturally sound because:
- The endpoint is used through a centralized API service (
getSitesSummaryApi) - No components are directly relying on pre-filtered online status in the response
- The change aligns with the separation of concerns, letting client components handle their own filtering needs
The implementation maintains proper error handling and authorization, while the architectural change provides more flexibility for different components to apply their own filtering logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other components that might be affected by this change
# Search for references to the sites summary endpoint
rg -l "devices/sites/summary" --type js --type jsx --type ts --type tsx
# Search for components that might be relying on pre-filtered online status
rg -l "online_status.*online" --type js --type jsx --type ts --type tsx
Length of output: 201
Script:
#!/bin/bash
# Let's try a different approach using more basic commands
# Search for references to the sites summary endpoint
rg "devices/sites/summary" -g "*.{js,ts,tsx}"
# Search for components that might be relying on pre-filtered online status
rg "online_status.*online" -g "*.{js,ts,tsx}"
# Also check for the specific URL
rg "analytics.airqo.net/api/v2/devices/sites/summary" -g "*.{js,ts,tsx}"
Length of output: 514
Script:
#!/bin/bash
# Let's check how this URL is being used in the codebase
# Check the analytics.js file to understand the URL usage
cat platform/src/core/urls/analytics.js
# Look for imports of SITES_SUMMARY_URL
rg "SITES_SUMMARY_URL" -g "*.{js,ts,tsx}"
# Check for any query parameter handling related to online_status
rg -B2 -A2 "SITES_SUMMARY_URL" -g "*.{js,ts,tsx}"
Length of output: 1721
Script:
#!/bin/bash
# Let's check how getSitesSummaryApi is being used
# Look for usages of getSitesSummaryApi
rg -B2 -A2 "getSitesSummaryApi" -g "*.{js,ts,tsx}"
# Also check for any components that might be handling site filtering
rg "filterSites|filteredSites" -g "*.{js,ts,tsx}"
Length of output: 3546
platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx (2)
230-230: LGTM! Consistent use of filtered sites
The update to use filteredSites in the DataTable component maintains consistency with the PR's objective of showing only online sites.
Line range hint 52-230: Verify the impact on user experience
The changes successfully implement site filtering at the component level, but let's verify the user experience.
✅ Verification successful
Implementation maintains consistent UX and aligns with existing patterns
The filtering implementation is safe and consistent with the codebase's existing patterns:
- The
isOnline === truefilter is already used in other components (e.g., Preferences.js) - Error handling and loading states are properly maintained in the Modal components
- The filtering logic is isolated to this component and doesn't affect other site status displays
- The implementation follows the same pattern as other components in the Modal directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential UX impact by analyzing related components and tests
# Look for any existing tests that might need updates
rg -A 5 "test\(.*AddLocations"
# Check for other components that might be affected by the filtering change
rg -A 5 "sitesSummaryData.*online_status"
# Look for any error handling or loading states related to site filtering
ast-grep --pattern 'loading || error'
Length of output: 116
Script:
#!/bin/bash
# Let's try a different approach to verify the UX impact
# Check for test files related to AddLocations
fd "AddLocations.*test" --type f
# Look for components that might be consuming the filtered sites
rg -A 3 "filteredSites.*="
# Check for any existing filtering logic for online sites
rg -A 3 "isOnline.*=.*true"
# Look for any UI components that display site status
rg -A 3 "site\.isOnline"
# Check for any error messages or loading states in the Modal directory
fd . "platform/src/common/components/Modal" -t f -x rg -l "loading|error"
Length of output: 4694
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
platform/src/core/utils/useFetchAnalyticsData.jsx (1)
87-87: Limit logging of error details to prevent exposing sensitive informationLogging entire error objects may inadvertently expose sensitive data. Consider logging only the error message or relevant information.
Apply this diff to refine the logging:
-console.error('Error fetching analytics data:', err); +console.error('Error fetching analytics data:', err.message);
| const token = localStorage.getItem('token'); | ||
| if (!token) throw new Error('Authorization token is missing.'); |
There was a problem hiding this comment.
Avoid storing tokens in localStorage for security reasons
Storing authentication tokens in localStorage can expose them to XSS attacks. Consider using secure cookies (with HttpOnly and Secure flags) or leveraging more secure storage mechanisms like Redux or Context API to enhance security.
| if (response.status === 200 && response.data?.status === 'success') { | ||
| return response.data.data || []; | ||
| } else { | ||
| throw new Error( | ||
| response.data?.message || 'Failed to fetch analytics data.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent response handling in fetchAnalytics
The response handling logic differs between development and production environments. In development, you're checking:
if (response.status === 200 && response.data?.status === 'success')While in production, you have:
if (response.status === 'success' && Array.isArray(response.data))Align the response validation criteria to ensure consistent behavior across environments, which will help avoid unexpected issues when switching between development and production.
Also applies to: 34-38
| const data = await fetchAnalytics(requestBody, token, signal); | ||
| setAllSiteData(data); | ||
| } catch (err) { | ||
| if (err.name === 'CanceledError' || err.name === 'AbortError') return; // Request was cancelled |
There was a problem hiding this comment.
Update Axios cancellation error handling
When handling canceled requests with Axios, the error code is 'ERR_CANCELED' or you can use axios.isCancel(err). Modify the error check to accurately detect canceled requests.
Apply this diff to update the error handling:
-if (err.name === 'CanceledError' || err.name === 'AbortError') return; // Request was cancelled
+if (err.code === 'ERR_CANCELED' || axios.isCancel(err)) return; // Request was cancelled📝 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 (err.name === 'CanceledError' || err.name === 'AbortError') return; // Request was cancelled | |
| if (err.code === 'ERR_CANCELED' || axios.isCancel(err)) return; // Request was cancelled |
|
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):
Screenshots (optional)
Summary by CodeRabbit
New Features
Bug Fixes
Chores