Update sites and devices to use Cohorts APIs in data download in analytics #2975
Conversation
📝 WalkthroughWalkthroughConsolidates device/site pagination into cohort-based fetching using groupId and device category; adds cohort APIs and URLs, forwards new DataTable prop Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as DataDownload UI
participant Hook as usePaginatedDevicesSummary
participant Account as Account.getGroupCohortsApi
participant RegistryDev as DeviceRegistry.getDevicesForCohortsApi
participant RegistrySites as DeviceRegistry.getSitesForCohortsApi
participant Cache as SWR Cache
User->>UI: Open Download Insights
UI->>Hook: init({ group: groupId, category, search, pagination })
Hook->>Cache: lookup(['devices-summary-paginated', groupId||'all', category, search||''])
alt cache miss
Hook->>Account: getGroupCohortsApi(groupId, signal)
Account-->>Hook: cohort_ids
alt cohorts found
par fetch devices and sites
Hook->>RegistryDev: getDevicesForCohortsApi({ cohort_ids, limit, skip, category, search, signal })
RegistryDev-->>Hook: { data: devices, meta }
Hook->>RegistrySites: getSitesForCohortsApi({ cohort_ids, limit, skip, category, search, signal })
RegistrySites-->>Hook: { data: sites, meta }
end
Hook->>Cache: set(key, { data, meta })
else no cohorts
Hook-->>Cache: set(key, { data: [], meta: {} })
end
else cache hit
Cache-->>Hook: cached { data, meta }
end
Hook-->>UI: { data, isLoading, isError, error, meta, loadMore, nextPage, prevPage }
UI-->>User: Render tables, banners (includes gas), pagination, errors (via getApiErrorMessage)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (1)
336-343: Bug: wrong variable passed to getApiErrorMessageYou’re passing
siteAndDeviceIds(data) instead of the error object, so the message is unreliable.const { data: siteAndDeviceIds, isLoading: isLoadingSiteIds, isError: isSiteIdsError, + error: siteIdsErrorMsg, } = useSiteAndDeviceIds(selectedGridId); @@ } else if (isSiteIdsError) { - setStatusMessage(getApiErrorMessage(siteAndDeviceIds) || 'Error loading data. Please try again.'); + setStatusMessage( + getApiErrorMessage(siteIdsErrorMsg) || 'Error loading data. Please try again.', + ); setMessageType(MESSAGE_TYPES.ERROR);
🧹 Nitpick comments (6)
src/platform/src/common/features/download-insights-locations/data-download/components/DataContent.js (1)
187-196: Forwarding activeFilterKey — good; drop unused pagination props to reduce noiseDataTable doesn’t consume enableInfiniteScroll, canLoadMore, or hasNextPage (it destructures underscored variants). Passing these can confuse readers.
Apply this diff:
defaultSortDirection="asc" activeFilterKey={activeFilterKey} // Pagination props - enableInfiniteScroll={enableInfiniteScroll} paginationMeta={paginationMeta} onLoadMore={onLoadMore} onNextPage={onNextPage} onPrevPage={onPrevPage} - canLoadMore={canLoadMore} - hasNextPage={hasNextPage}src/platform/src/lib/constants.js (1)
21-30: Freeze IDs and consider env-backed configIDs aren’t secrets, but freezing avoids accidental mutation. Longer-term, allow override via env to avoid code changes when IDs change.
Apply this diff:
-export const AIRQO_GROUP_IDS = { +export const AIRQO_GROUP_IDS = Object.freeze({ production: '64f54e4621d9b90013925a08', staging: '653b00efb657380014328b54', -}; +});Optional (future): read from env with sane fallbacks, e.g., process.env.NEXT_PUBLIC_AIRQO_GROUP_ID_PROD/STAGING.
src/platform/src/core/apis/Account.js (1)
252-270: Cohorts API helper — add timeout and normalized error messageFunction is solid. Consider a timeout and consistent error surface for better UX.
Apply this diff:
export const getGroupCohortsApi = (groupId, signal) => { if (!groupId) { return Promise.reject(new Error('Group ID is required')); } return secureApiProxy - .get(getGroupCohortsUrl(groupId), { authType: AUTH_TYPES.JWT, signal }) - .then((response) => response.data) - .catch((error) => { - throw error; - }); + .get(getGroupCohortsUrl(groupId), { + authType: AUTH_TYPES.JWT, + signal, + timeout: 15000, + }) + .then((response) => response.data) + .catch((error) => { + const msg = error?.response?.data?.message || error.message || 'Failed to fetch cohorts'; + throw new Error(msg); + }); };src/platform/src/core/utils/loginSetup.js (1)
261-268: Augment fallback group with grp_slug for broader compatibilitySeveral places check organization_slug || grp_slug. Adding grp_slug improves resilience.
Apply this diff:
userGroups = [ { _id: getAirqoGroupId(), grp_title: 'airqo', grp_name: 'airqo', organization_slug: 'airqo', + grp_slug: 'airqo', }, ];src/platform/src/core/hooks/usePaginatedData.js (1)
347-392: Cohort-driven fetcher looks good; consider caching cohort IDs per groupCurrent flow fetches cohorts on every page/search change. Cache cohort IDs per group (e.g., a separate SWR call keyed by group) and reuse them to cut round-trips and latency while paginating.
Also, until DeviceRegistry forwards
search, devices search will be a no-op. See suggested fix in DeviceRegistry.src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (1)
660-666: Nit: redundant checks in next/prev handlersThe inner
if (formData.deviceCategory)branches return the same value.const currentNextPage = useMemo(() => { if (activeFilterKey === FILTER_TYPES.DEVICES) { - if (formData.deviceCategory) { - return devicesNextPage; - } - return devicesNextPage; + return devicesNextPage; } const nextMap = { [FILTER_TYPES.COUNTRIES]: countriesNextPage, [FILTER_TYPES.CITIES]: citiesNextPage, [FILTER_TYPES.SITES]: sitesNextPage, }; return nextMap[activeFilterKey]; }, [ activeFilterKey, - formData.deviceCategory, countriesNextPage, citiesNextPage, devicesNextPage, sitesNextPage, ]); const currentPrevPage = useMemo(() => { if (activeFilterKey === FILTER_TYPES.DEVICES) { - if (formData.deviceCategory) { - return devicesPrevPage; - } - return devicesPrevPage; + return devicesPrevPage; } const prevMap = { [FILTER_TYPES.COUNTRIES]: countriesPrevPage, [FILTER_TYPES.CITIES]: citiesPrevPage, [FILTER_TYPES.SITES]: sitesPrevPage, }; return prevMap[activeFilterKey]; }, [ activeFilterKey, - formData.deviceCategory, countriesPrevPage, citiesPrevPage, devicesPrevPage, sitesPrevPage, ]);Also applies to: 684-689
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/platform/src/common/features/download-insights-locations/components/DataTable.jsx(2 hunks)src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx(17 hunks)src/platform/src/common/features/download-insights-locations/data-download/components/DataContent.js(1 hunks)src/platform/src/common/features/download-insights-locations/data-download/hooks/useDataDownloadLogic.js(1 hunks)src/platform/src/core/apis/Account.js(2 hunks)src/platform/src/core/apis/DeviceRegistry.js(2 hunks)src/platform/src/core/hooks/analyticHooks.js(0 hunks)src/platform/src/core/hooks/usePaginatedData.js(1 hunks)src/platform/src/core/urls/authentication.js(1 hunks)src/platform/src/core/urls/deviceRegistry.js(1 hunks)src/platform/src/core/utils/loginSetup.js(2 hunks)src/platform/src/lib/constants.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/platform/src/core/hooks/analyticHooks.js
🧰 Additional context used
🧬 Code graph analysis (7)
src/platform/src/core/utils/loginSetup.js (3)
src/platform/src/app/providers/UnifiedGroupProvider.jsx (1)
userGroups(153-171)src/platform/src/core/utils/domainUpdateVerification.js (1)
userGroups(14-15)src/platform/src/lib/constants.js (2)
getAirqoGroupId(26-30)getAirqoGroupId(26-30)
src/platform/src/common/features/download-insights-locations/components/DataTable.jsx (2)
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (1)
filters(554-569)src/platform/src/common/features/download-insights-locations/data-download/hooks/useDataDownloadLogic.js (1)
activeFilterKey(109-109)
src/platform/src/core/apis/Account.js (2)
src/platform/src/core/utils/secureApiProxyClient.js (4)
secureApiProxy(344-344)secureApiProxy(344-344)AUTH_TYPES(93-98)AUTH_TYPES(93-98)src/platform/src/core/urls/authentication.js (2)
getGroupCohortsUrl(28-28)getGroupCohortsUrl(28-28)
src/platform/src/core/apis/DeviceRegistry.js (3)
src/platform/src/core/hooks/usePaginatedData.js (2)
skip(43-43)limit(35-35)src/platform/src/core/utils/secureApiProxyClient.js (4)
AUTH_TYPES(93-98)AUTH_TYPES(93-98)secureApiProxy(344-344)secureApiProxy(344-344)src/platform/src/core/urls/deviceRegistry.js (2)
DEVICES_FOR_COHORTS_URL(5-5)DEVICES_FOR_COHORTS_URL(5-5)
src/platform/src/core/hooks/usePaginatedData.js (2)
src/platform/src/core/apis/Account.js (3)
params(325-325)getGroupCohortsApi(259-269)getGroupCohortsApi(259-269)src/platform/src/core/apis/DeviceRegistry.js (7)
params(47-47)params(97-102)params(190-190)params(215-215)params(239-239)getDevicesForCohortsApi(85-130)getDevicesForCohortsApi(85-130)
src/platform/src/common/features/download-insights-locations/data-download/components/DataContent.js (1)
src/platform/src/common/features/download-insights-locations/data-download/hooks/useDataDownloadLogic.js (1)
activeFilterKey(109-109)
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (2)
src/platform/src/app/providers/UnifiedGroupProvider.jsx (2)
useGetActiveGroup(850-879)useGetActiveGroup(850-879)src/platform/src/core/hooks/usePaginatedData.js (2)
usePaginatedDevicesSummary(347-393)usePaginatedDevicesSummary(347-393)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-push-deploy-analytics-platform-preview
🔇 Additional comments (5)
src/platform/src/core/urls/deviceRegistry.js (1)
5-5: Cohort devices URL added — looks goodConstant name and path are clear and consistent with the file’s style.
src/platform/src/core/urls/authentication.js (1)
27-29: New group cohorts URL helper — LGTMStraightforward helper aligned with existing patterns.
src/platform/src/core/utils/loginSetup.js (1)
28-28: Import of getAirqoGroupId — good centralizationNice consolidation of environment-specific group IDs.
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (1)
272-290: LGTM: unified devices pagination via groupId + categoryGood consolidation to usePaginatedDevicesSummary with groupId and category. Aligns with the cohorts-first flow and simplifies pagination state.
src/platform/src/core/apis/DeviceRegistry.js (1)
85-131: Forwardsearchand surface axios cancellations ingetDevicesForCohortsApi
- In
src/platform/src/core/apis/DeviceRegistry.js, extendgetDevicesForCohortsApito forward a non-emptysearchparam intoparams.- Remove the
.catchso that axios’sCanceledError/ERR_CANCELEDpropagate to SWR’s abort logic.- Confirm the backend supports
searchonDEVICES_FOR_COHORTS_URL.
src/platform/src/common/features/download-insights-locations/components/DataTable.jsx
Show resolved
Hide resolved
|
|
||
| // Ensure deviceCategoryValue is valid | ||
| const validCategories = ['lowcost', 'bam', 'mobile']; | ||
| const validCategories = ['lowcost', 'bam', 'mobile', 'gas']; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
LGTM! Verify backend API support for 'gas' category.
The addition of 'gas' to the valid device categories is correct and aligns with the PR's goal of supporting cohorts APIs. This prevents 'gas' devices from being incorrectly coerced to 'lowcost'.
Please verify that the backend API accepts 'gas' as a valid device_category parameter. Run the following script to check for other locations in the codebase that might also need to support the 'gas' category:
🏁 Script executed:
#!/bin/bash
# Description: Find other references to device categories that may need updating
# Search for device category validation or constants
rg -nP --type=js --type=jsx --type=ts --type=tsx -C3 '\b(lowcost|bam|mobile)\b.*\b(lowcost|bam|mobile)\b' \
-g '!node_modules' -g '!dist' -g '!build'
# Search for device_category or deviceCategory field usage
rg -nP --type=js --type=jsx --type=ts --type=tsx -C3 'device[_-]?category' \
-g '!node_modules' -g '!dist' -g '!build'Length of output: 55
🏁 Script executed:
#!/bin/bash
# Description: Find other references to device categories that may need updating
# Search for device category validation or constants
rg -nP --type js --type ts -C3 '\b(lowcost|bam|mobile)\b.*\b(lowcost|bam|mobile)\b' \
-g '!node_modules' -g '!dist' -g '!build'
# Search for device_category or deviceCategory field usage
rg -nP --type js --type ts -C3 'device[_-]?category' \
-g '!node_modules' -g '!dist' -g '!build'Length of output: 10619
Extend 'gas' support across device-category logic
- DataDownload.jsx: update raw/calibrated and frequency filters to include ‘gas’
- useAnalyticsDownload.js & useDownload.js: replace hardcoded
'lowcost'with dynamicdeviceCategoryValue - DataContent.js: add messaging for ‘gas’ devices
- useDataPreview.js: ensure preview payload handles ‘gas’
- Verify backend API accepts ‘gas’ as a valid
device_category
🤖 Prompt for AI Agents
In
src/platform/src/common/features/download-insights-locations/data-download/hooks/useDataDownloadLogic.js
around line 494, the device-category logic must fully support the 'gas' device
type: ensure the validCategories array explicitly includes 'gas' (if not
already), replace any hardcoded 'lowcost' references in this file with the
dynamic deviceCategoryValue variable, and extend the raw/calibrated and
frequency filter checks to include 'gas' alongside other categories so filters
and payloads are generated correctly for gas devices.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/platform/src/common/features/download-insights-locations/components/DataTable.jsx(3 hunks)src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx(17 hunks)src/platform/src/common/features/download-insights-locations/data-download/constants.js(1 hunks)src/platform/src/core/hooks/usePaginatedData.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/platform/src/core/hooks/usePaginatedData.js (2)
src/platform/src/core/apis/Account.js (3)
params(325-325)getGroupCohortsApi(259-269)getGroupCohortsApi(259-269)src/platform/src/core/apis/DeviceRegistry.js (7)
params(47-47)params(97-102)params(190-190)params(215-215)params(239-239)getDevicesForCohortsApi(85-130)getDevicesForCohortsApi(85-130)
src/platform/src/common/features/download-insights-locations/components/DataTable.jsx (3)
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (1)
filters(553-568)src/platform/src/common/features/download-insights-locations/add-favorites/components/MainContent.jsx (1)
filters(42-45)src/platform/src/common/features/download-insights-locations/data-download/hooks/useDataDownloadLogic.js (1)
activeFilterKey(109-109)
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (2)
src/platform/src/app/providers/UnifiedGroupProvider.jsx (2)
useGetActiveGroup(850-879)useGetActiveGroup(850-879)src/platform/src/core/hooks/usePaginatedData.js (2)
usePaginatedDevicesSummary(347-392)usePaginatedDevicesSummary(347-392)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-push-deploy-analytics-platform-preview
🔇 Additional comments (3)
src/platform/src/common/features/download-insights-locations/components/DataTable.jsx (1)
313-313: Previous feedback successfully addressed.The
activeFilterKeyprop and synchronization logic have been implemented as suggested in the previous review. The state now properly updates when the prop or filters change, and stale column filters and pagination are reset.Also applies to: 326-330, 354-362
src/platform/src/common/features/download-insights-locations/data-download/constants.js (1)
32-32: LGTM.The new
gasdevice category option is consistent with the existing entries and properly integrated.src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (1)
54-54: Improved error handling with centralized formatting.The introduction of
getApiErrorMessageprovides consistent, user-friendly error messages across all data loading scenarios. This is a solid improvement over the previous raw error handling.Also applies to: 306-326, 1293-1309
| const { | ||
| data: mobileDevicesData, | ||
| isLoading: mobileDevicesLoading, | ||
| isError: mobileDevicesError, | ||
| error: mobileDevicesErrorMsg, | ||
| refresh: refreshMobileDevices, | ||
| meta: mobileDevicesMeta, | ||
| loadMore: loadMoreMobileDevices, | ||
| canLoadMore: canLoadMoreMobileDevices, | ||
| hasNextPage: mobileDevicesHasNextPage, | ||
| nextPage: mobileDevicesNextPage, | ||
| prevPage: mobileDevicesPrevPage, | ||
| } = usePaginatedMobileDevices({ | ||
| enableInfiniteScroll: false, | ||
| initialLimit: 6, | ||
| search: searchQuery, | ||
| group: groupTitle, | ||
| }); | ||
|
|
||
| const { | ||
| data: bamDevicesData, | ||
| isLoading: bamDevicesLoading, | ||
| isError: bamDevicesError, | ||
| error: bamDevicesErrorMsg, | ||
| refresh: refreshBAMDevices, | ||
| meta: bamDevicesMeta, | ||
| loadMore: loadMoreBAMDevices, | ||
| canLoadMore: canLoadMoreBAMDevices, | ||
| hasNextPage: bamDevicesHasNextPage, | ||
| nextPage: bamDevicesNextPage, | ||
| prevPage: bamDevicesPrevPage, | ||
| } = usePaginatedBAMDevices({ | ||
| enableInfiniteScroll: false, | ||
| initialLimit: 6, | ||
| search: searchQuery, | ||
| group: groupTitle, | ||
| }); | ||
|
|
||
| const { | ||
| data: lowCostDevicesData, | ||
| isLoading: lowCostDevicesLoading, | ||
| isError: lowCostDevicesError, | ||
| error: lowCostDevicesErrorMsg, | ||
| refresh: refreshLowCostDevices, | ||
| meta: lowCostDevicesMeta, | ||
| loadMore: loadMoreLowCostDevices, | ||
| canLoadMore: canLoadMoreLowCostDevices, | ||
| hasNextPage: lowCostDevicesHasNextPage, | ||
| nextPage: lowCostDevicesNextPage, | ||
| prevPage: lowCostDevicesPrevPage, | ||
| } = usePaginatedLowCostDevices({ | ||
| data: devicesData, | ||
| isLoading: devicesLoading, | ||
| isError: devicesError, | ||
| error: devicesErrorMsg, | ||
| refresh: refreshDevices, | ||
| meta: devicesMeta, | ||
| loadMore: loadMoreDevices, | ||
| canLoadMore: canLoadMoreDevices, | ||
| hasNextPage: devicesHasNextPage, | ||
| nextPage: devicesNextPage, | ||
| prevPage: devicesPrevPage, | ||
| } = usePaginatedDevicesSummary(groupId, { | ||
| enableInfiniteScroll: false, | ||
| initialLimit: 6, | ||
| search: searchQuery, | ||
| group: groupTitle, | ||
| category: formData.deviceCategory?.name?.toLowerCase() || 'lowcost', | ||
| }); |
There was a problem hiding this comment.
Critical: Function signature mismatch.
The call to usePaginatedDevicesSummary on line 284 passes groupId as the first argument, but the hook's new signature expects a single options object. This mismatch will cause groupId to be destructured as options, leading to incorrect behavior.
Apply this fix:
const {
data: devicesData,
isLoading: devicesLoading,
isError: devicesError,
error: devicesErrorMsg,
refresh: refreshDevices,
meta: devicesMeta,
loadMore: loadMoreDevices,
canLoadMore: canLoadMoreDevices,
hasNextPage: devicesHasNextPage,
nextPage: devicesNextPage,
prevPage: devicesPrevPage,
- } = usePaginatedDevicesSummary(groupId, {
+ } = usePaginatedDevicesSummary({
+ group: groupId,
enableInfiniteScroll: false,
initialLimit: 6,
search: searchQuery,
category: formData.deviceCategory?.name?.toLowerCase() || 'lowcost',
});📝 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 { | |
| data: mobileDevicesData, | |
| isLoading: mobileDevicesLoading, | |
| isError: mobileDevicesError, | |
| error: mobileDevicesErrorMsg, | |
| refresh: refreshMobileDevices, | |
| meta: mobileDevicesMeta, | |
| loadMore: loadMoreMobileDevices, | |
| canLoadMore: canLoadMoreMobileDevices, | |
| hasNextPage: mobileDevicesHasNextPage, | |
| nextPage: mobileDevicesNextPage, | |
| prevPage: mobileDevicesPrevPage, | |
| } = usePaginatedMobileDevices({ | |
| enableInfiniteScroll: false, | |
| initialLimit: 6, | |
| search: searchQuery, | |
| group: groupTitle, | |
| }); | |
| const { | |
| data: bamDevicesData, | |
| isLoading: bamDevicesLoading, | |
| isError: bamDevicesError, | |
| error: bamDevicesErrorMsg, | |
| refresh: refreshBAMDevices, | |
| meta: bamDevicesMeta, | |
| loadMore: loadMoreBAMDevices, | |
| canLoadMore: canLoadMoreBAMDevices, | |
| hasNextPage: bamDevicesHasNextPage, | |
| nextPage: bamDevicesNextPage, | |
| prevPage: bamDevicesPrevPage, | |
| } = usePaginatedBAMDevices({ | |
| enableInfiniteScroll: false, | |
| initialLimit: 6, | |
| search: searchQuery, | |
| group: groupTitle, | |
| }); | |
| const { | |
| data: lowCostDevicesData, | |
| isLoading: lowCostDevicesLoading, | |
| isError: lowCostDevicesError, | |
| error: lowCostDevicesErrorMsg, | |
| refresh: refreshLowCostDevices, | |
| meta: lowCostDevicesMeta, | |
| loadMore: loadMoreLowCostDevices, | |
| canLoadMore: canLoadMoreLowCostDevices, | |
| hasNextPage: lowCostDevicesHasNextPage, | |
| nextPage: lowCostDevicesNextPage, | |
| prevPage: lowCostDevicesPrevPage, | |
| } = usePaginatedLowCostDevices({ | |
| data: devicesData, | |
| isLoading: devicesLoading, | |
| isError: devicesError, | |
| error: devicesErrorMsg, | |
| refresh: refreshDevices, | |
| meta: devicesMeta, | |
| loadMore: loadMoreDevices, | |
| canLoadMore: canLoadMoreDevices, | |
| hasNextPage: devicesHasNextPage, | |
| nextPage: devicesNextPage, | |
| prevPage: devicesPrevPage, | |
| } = usePaginatedDevicesSummary(groupId, { | |
| enableInfiniteScroll: false, | |
| initialLimit: 6, | |
| search: searchQuery, | |
| group: groupTitle, | |
| category: formData.deviceCategory?.name?.toLowerCase() || 'lowcost', | |
| }); | |
| const { | |
| data: devicesData, | |
| isLoading: devicesLoading, | |
| isError: devicesError, | |
| error: devicesErrorMsg, | |
| refresh: refreshDevices, | |
| meta: devicesMeta, | |
| loadMore: loadMoreDevices, | |
| canLoadMore: canLoadMoreDevices, | |
| hasNextPage: devicesHasNextPage, | |
| nextPage: devicesNextPage, | |
| prevPage: devicesPrevPage, | |
| } = usePaginatedDevicesSummary({ | |
| group: groupId, | |
| enableInfiniteScroll: false, | |
| initialLimit: 6, | |
| search: searchQuery, | |
| category: formData.deviceCategory?.name?.toLowerCase() || 'lowcost', | |
| }); |
🤖 Prompt for AI Agents
In
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx
around lines 272 to 289, the hook is being called with groupId as a positional
first argument but the new usePaginatedDevicesSummary signature expects a single
options object; fix this by replacing the positional call with a single object
containing groupId and the existing options (e.g. { groupId,
enableInfiniteScroll: false, initialLimit: 6, search: searchQuery, category:
formData.deviceCategory?.name?.toLowerCase() || 'lowcost' }) so the hook
receives the correct properties.
| export const usePaginatedDevicesSummary = (options = {}) => { | ||
| const { search = '', group = '', category = 'lowcost', ...rest } = options; | ||
| const normalizedGroup = | ||
| typeof group === 'string' && group.trim().length > 0 ? group.trim() : ''; | ||
|
|
||
| const fetcher = useCallback( | ||
| async (params, signal) => { | ||
| const { getDeviceSummaryApi } = await import('../apis/Analytics'); | ||
| const groupParam = normalizedGroup || undefined; | ||
| return getDeviceSummaryApi({ | ||
| group: groupParam, | ||
| status: 'deployed', | ||
| skip: params.skip, | ||
| limit: params.limit, | ||
| search: params.search, | ||
| if (!normalizedGroup) return { devices: [], meta: {} }; | ||
|
|
||
| const { getGroupCohortsApi } = await import('../apis/Account'); | ||
| const { getDevicesForCohortsApi } = await import('../apis/DeviceRegistry'); | ||
|
|
||
| // Step 1: Fetch cohorts for the group | ||
| const cohortsResponse = await getGroupCohortsApi(normalizedGroup, signal); | ||
| const cohortIds = cohortsResponse?.data || []; | ||
|
|
||
| if (cohortIds.length === 0) { | ||
| return { devices: [], meta: {} }; | ||
| } | ||
|
|
||
| // Step 2: Fetch devices for the retrieved cohorts | ||
| return getDevicesForCohortsApi( | ||
| { | ||
| cohort_ids: cohortIds, | ||
| skip: params.skip, | ||
| limit: params.limit, | ||
| category: category, | ||
| ...(params.search && { search: params.search }), | ||
| }, | ||
| signal, | ||
| }); | ||
| ); | ||
| }, | ||
| [normalizedGroup], | ||
| [normalizedGroup, category], | ||
| ); | ||
|
|
||
| return usePaginatedData( | ||
| ['devices-summary-paginated', normalizedGroup || 'all', search || ''], | ||
| ['devices-summary-paginated', normalizedGroup || 'all', category, search || ''], | ||
| fetcher, | ||
| { | ||
| initialLimit: 20, | ||
| maxLimit: 80, | ||
| ...restOptions, | ||
| ...rest, | ||
| search, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify cohort API error handling.
The refactored hook now uses a two-step flow (fetch cohorts, then devices), which is a significant architectural change. However, if the cohort API fails, the error will propagate through SWR without explicit handling. Consider whether partial failures need special treatment.
Also, when normalizedGroup is empty, the hook returns early with empty data. Ensure this behavior aligns with expectations for unauthenticated or group-less scenarios.
Verify that the cohort API failure scenarios are properly handled by SWR's error handling mechanism:
🏁 Script executed:
#!/bin/bash
# Check how cohort API errors are handled in consuming components
rg -n -C3 'usePaginatedDevicesSummary' src/platform/src/common/features/download-insights-locations/Length of output: 1717
Update hook usage signature and verify error handling
- DataDownload.jsx (line 284) and any other consumers must call
usePaginatedDevicesSummary({ group: groupId, enableInfiniteScroll: false, initialLimit: 6, search: searchQuery })instead of positional args. - Ensure the fetcher’s two-step flow handles cohort API failures appropriately (errors currently bubble through SWR).
- Confirm that early return with empty
groupaligns with expected behavior for group-less scenarios.
|
New next-platform changes available for preview here |
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx(18 hunks)src/platform/src/core/apis/DeviceRegistry.js(2 hunks)src/platform/src/core/hooks/usePaginatedData.js(2 hunks)src/platform/src/core/urls/deviceRegistry.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/platform/src/core/apis/DeviceRegistry.js
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-22T07:30:46.957Z
Learnt from: Codebmk
PR: airqo-platform/AirQo-frontend#2944
File: src/vertex/components/features/dashboard/stats-cards.tsx:108-114
Timestamp: 2025-09-22T07:30:46.957Z
Learning: In the useDeviceCount hook and getDeviceCountApi, the groupId parameter expects the group title (grp_title) not the group's _id field. The API maps groupId to the group_id query parameter which uses the group name/title string value.
Applied to files:
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx
📚 Learning: 2025-09-22T07:30:46.957Z
Learnt from: Codebmk
PR: airqo-platform/AirQo-frontend#2944
File: src/vertex/components/features/dashboard/stats-cards.tsx:108-114
Timestamp: 2025-09-22T07:30:46.957Z
Learning: In the useDeviceCount hook and related device APIs, the groupId parameter expects the group title (grp_title) not the group's _id field. The API maps groupId to the group_id query parameter which uses the group name/title.
Applied to files:
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx
🧬 Code graph analysis (2)
src/platform/src/core/hooks/usePaginatedData.js (3)
src/platform/src/core/apis/DeviceRegistry.js (10)
params(48-48)params(99-104)params(147-152)params(232-232)params(257-257)params(281-281)getSitesForCohortsApi(134-172)getSitesForCohortsApi(134-172)getDevicesForCohortsApi(86-132)getDevicesForCohortsApi(86-132)src/platform/src/core/apis/Account.js (3)
params(325-325)getGroupCohortsApi(259-269)getGroupCohortsApi(259-269)src/platform/src/core/apis/Analytics.js (5)
params(38-45)params(69-77)params(100-107)getSitesSummaryApi(31-59)getSitesSummaryApi(31-59)
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (2)
src/platform/src/app/providers/UnifiedGroupProvider.jsx (2)
useGetActiveGroup(850-879)useGetActiveGroup(850-879)src/platform/src/core/hooks/usePaginatedData.js (4)
usePaginatedSitesSummary(312-367)usePaginatedSitesSummary(312-367)usePaginatedDevicesSummary(372-417)usePaginatedDevicesSummary(372-417)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-push-deploy-analytics-platform-preview
🔇 Additional comments (6)
src/platform/src/core/urls/deviceRegistry.js (1)
5-7: LGTM!The new URL constants follow the established naming conventions and fit naturally into the existing URL structure.
src/platform/src/core/hooks/usePaginatedData.js (3)
317-355: Verify UX for empty cohorts.When a group has no cohorts (line 333-335), the hook returns empty data without an error. This could confuse users who expect to see sites for their group but get nothing. Consider whether this scenario warrants a specific error message or warning.
Additionally, if
getGroupCohortsApifails, the error propagates through SWR without explicit handling. Confirm this aligns with your error-handling strategy.
407-416: Good: SWR key includes category.The SWR key (line 408) correctly includes the
categoryparameter, ensuring data is refetched when the device category changes. This prevents serving stale data across different device types.
377-405: Breaking change: devices hook lacks fallback when group is empty.
Unlike usePaginatedSitesSummary, usePaginatedDevicesSummary immediately returns{ devices: [], meta: {} }whengroupis empty. Confirm this is intentional and ensure consumers handle the empty state; if devices should be available without a group, add a fallback fetch path similar to the sites implementation.src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (2)
80-80: Good: using group _id instead of title.Extracting
idasgroupIdaligns with the learnings that state API parameters expect the group's_idfield, not the title. This corrects potential issues from using the group title as an identifier.Based on learnings.
272-289: Hook call signature corrected.The call to
usePaginatedDevicesSummarynow correctly passesgroupIdas the first positional argument with options as the second parameter, matching the hook's signature(group, options = {}). This addresses the critical issue flagged in previous reviews.
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx
Outdated
Show resolved
Hide resolved
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (2)
208-209: Good fix: pass the hook error to getApiErrorMessage.Destructuring error as siteAndDeviceIdsError and using it in the status message resolves the earlier issue of passing data instead of the error.
Also applies to: 340-341, 349-357
274-290: Hook call signature is correct; prior bot warning is outdated.usePaginatedDevicesSummary currently takes (group, options). Your call with groupId as first arg and options object is correct per core/hooks/usePaginatedData.
🧹 Nitpick comments (4)
src/platform/src/core/hooks/usePaginatedData.js (1)
373-373: Consider documenting expected category values.The
categoryparameter defaults to'lowcost'but lacks validation or documentation. With the addition of new categories like'gas'(per the AI summary), consider documenting the expected values or adding runtime validation to catch invalid categories early.src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (3)
305-327: Optional: prune unused dependency to avoid extra effect runs.backToDownload isn’t used inside this effect. Remove it from the deps array.
useEffect(() => { setFilterErrors({ sites: sitesError ? getApiErrorMessage(sitesErrorMsg) || 'Error loading sites' : '', devices: devicesError ? getApiErrorMessage(devicesErrorMsg) || 'Error loading devices' : '', countries: countriesError ? getApiErrorMessage(countriesErrorMsg) || 'Error loading countries' : '', cities: citiesError ? getApiErrorMessage(citiesErrorMsg) || 'Error loading cities' : '', }); }, [ sitesError, sitesErrorMsg, devicesError, devicesErrorMsg, countriesError, countriesErrorMsg, citiesError, citiesErrorMsg, setFilterErrors, - backToDownload, ]);
662-667: Simplify redundant conditionals for devices next/prev.The inner if/else returns the same value; remove the dead branch.
const currentNextPage = useMemo(() => { if (activeFilterKey === FILTER_TYPES.DEVICES) { - if (formData.deviceCategory) { - return devicesNextPage; - } - return devicesNextPage; + return devicesNextPage; } ... }, [...]); const currentPrevPage = useMemo(() => { if (activeFilterKey === FILTER_TYPES.DEVICES) { - if (formData.deviceCategory) { - return devicesPrevPage; - } - return devicesPrevPage; + return devicesPrevPage; } ... }, [...]);Also applies to: 684-690
144-162: Handle the new ‘gas’ category in data type/frequency filters.If ‘gas’ devices are supported, reflect their constraints here (raw-only, etc.). Right now gas will fall through to defaults.
const filteredDataTypeOptions = useMemo(() => { const deviceCategory = formData.deviceCategory?.name?.toLowerCase() || 'lowcost'; if (deviceCategory === 'lowcost') { return DATA_TYPE_OPTIONS; } - if (deviceCategory === 'bam' || deviceCategory === 'mobile') { + if (deviceCategory === 'bam' || deviceCategory === 'mobile' || deviceCategory === 'gas') { return DATA_TYPE_OPTIONS.filter( (option) => option.name.toLowerCase() === 'raw data', ); } return DATA_TYPE_OPTIONS; }, [formData.deviceCategory]); const filteredFrequencyOptions = useMemo(() => { const deviceCategory = formData.deviceCategory?.name?.toLowerCase() || 'lowcost'; if ( activeFilterKey === FILTER_TYPES.DEVICES && - deviceCategory === 'mobile' + (deviceCategory === 'mobile' || deviceCategory === 'gas') ) { return FREQUENCY_OPTIONS.filter( (option) => option.name.toLowerCase() === 'raw', ); } if (activeFilterKey === FILTER_TYPES.DEVICES && deviceCategory === 'bam') { return FREQUENCY_OPTIONS; } return FREQUENCY_OPTIONS.filter( (option) => option.name.toLowerCase() !== 'raw', ); }, [formData.deviceCategory, activeFilterKey]);Also applies to: 165-187
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx(20 hunks)src/platform/src/core/hooks/usePaginatedData.js(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-22T07:30:46.957Z
Learnt from: Codebmk
PR: airqo-platform/AirQo-frontend#2944
File: src/vertex/components/features/dashboard/stats-cards.tsx:108-114
Timestamp: 2025-09-22T07:30:46.957Z
Learning: In the useDeviceCount hook and getDeviceCountApi, the groupId parameter expects the group title (grp_title) not the group's _id field. The API maps groupId to the group_id query parameter which uses the group name/title string value.
Applied to files:
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx
📚 Learning: 2025-09-22T07:30:46.957Z
Learnt from: Codebmk
PR: airqo-platform/AirQo-frontend#2944
File: src/vertex/components/features/dashboard/stats-cards.tsx:108-114
Timestamp: 2025-09-22T07:30:46.957Z
Learning: In the useDeviceCount hook and related device APIs, the groupId parameter expects the group title (grp_title) not the group's _id field. The API maps groupId to the group_id query parameter which uses the group name/title.
Applied to files:
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx
🧬 Code graph analysis (2)
src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (2)
src/platform/src/app/providers/UnifiedGroupProvider.jsx (2)
useGetActiveGroup(850-879)useGetActiveGroup(850-879)src/platform/src/core/hooks/usePaginatedData.js (4)
usePaginatedSitesSummary(312-367)usePaginatedSitesSummary(312-367)usePaginatedDevicesSummary(372-417)usePaginatedDevicesSummary(372-417)
src/platform/src/core/hooks/usePaginatedData.js (3)
src/platform/src/core/apis/DeviceRegistry.js (10)
params(48-48)params(99-104)params(147-152)params(232-232)params(257-257)params(281-281)getSitesForCohortsApi(134-172)getSitesForCohortsApi(134-172)getDevicesForCohortsApi(86-132)getDevicesForCohortsApi(86-132)src/platform/src/core/apis/Account.js (3)
params(325-325)getGroupCohortsApi(259-269)getGroupCohortsApi(259-269)src/platform/src/core/apis/Analytics.js (5)
params(38-45)params(69-77)params(100-107)getSitesSummaryApi(31-59)getSitesSummaryApi(31-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-push-deploy-analytics-platform-preview
🔇 Additional comments (7)
src/platform/src/core/hooks/usePaginatedData.js (4)
317-355: Cohort-based sites fetching implemented correctly.The two-step flow (fetch cohorts → fetch sites) is well-structured with proper signal handling in both API calls. The fallback to Analytics API when no group is provided maintains backward compatibility.
377-405: Critical signal placement issue resolved.The signal parameter is now correctly placed inside the options object (line 400), resolving the critical issue flagged in the previous review. The two-step cohort flow is well-implemented with proper signal handling and parameter passing.
408-408: SWR cache key correctly updated for category.Adding
categoryto the cache key ensures proper cache segregation, preventing data from different categories from being mixed. This is essential for the cohort-based fetching approach.
379-379: Confirm early-return behavior in hook consumers. The newif (!normalizedGroup)return (emptydevices/meta) is a breaking change—ensure DataDownload.jsx (line 285) and any otherusePaginatedDevicesSummaryconsumers handle the no-group scenario correctly.src/platform/src/common/features/download-insights-locations/data-download/DataDownload.jsx (3)
230-235: Sites hook call LGTM.Passing '' when grid-selection is active cleanly triggers the “all sites” fallback; revalidateOnMount is appropriate.
1295-1311: Error messaging path looks solid.Mapping API errors through getApiErrorMessage for the active filter with a sane fallback reads well.
80-81: Remove group identifier warning—passingidis correct for cohorts APIs. getGroupCohortsApi/getGroupCohortsUrl expect the group’s _id, so no change needed.Likely an incorrect or invalid review comment.
|
New next-platform changes available for preview here |
1 similar comment
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/platform/src/core/hooks/usePaginatedData.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/platform/src/core/hooks/usePaginatedData.js (3)
src/platform/src/core/apis/Account.js (3)
getGroupCohortsApi(259-269)getGroupCohortsApi(259-269)params(325-325)src/platform/src/core/apis/DeviceRegistry.js (10)
getSitesForCohortsApi(134-172)getSitesForCohortsApi(134-172)params(48-48)params(99-104)params(147-152)params(232-232)params(257-257)params(281-281)getDevicesForCohortsApi(86-132)getDevicesForCohortsApi(86-132)src/platform/src/core/apis/Analytics.js (5)
params(38-45)params(69-77)params(100-107)getSitesSummaryApi(31-59)getSitesSummaryApi(31-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-push-deploy-analytics-platform-preview
🔇 Additional comments (1)
src/platform/src/core/hooks/usePaginatedData.js (1)
5-5: Consistent error surfacing via getApiErrorMessage.Good addition; keeps API failure messages uniform across hooks.
| const { search = '', category = 'lowcost', ...rest } = options; | ||
| const normalizedGroup = | ||
| typeof group === 'string' && group.trim().length > 0 ? group.trim() : ''; | ||
|
|
||
| const fetcher = useCallback( | ||
| async (params, signal) => { | ||
| const { getDeviceSummaryApi } = await import('../apis/Analytics'); | ||
| const groupParam = normalizedGroup || undefined; | ||
| return getDeviceSummaryApi({ | ||
| group: groupParam, | ||
| status: 'deployed', | ||
| skip: params.skip, | ||
| limit: params.limit, | ||
| search: params.search, | ||
| signal, | ||
| if (normalizedGroup) { | ||
|
|
||
| const { getGroupCohortsApi } = await import('../apis/Account'); | ||
| const { getDevicesForCohortsApi } = await import('../apis/DeviceRegistry'); | ||
|
|
||
| // Step 1: Fetch cohorts for the group | ||
| try { | ||
| const cohortsResponse = await getGroupCohortsApi(normalizedGroup, signal); | ||
| const cohortIds = cohortsResponse?.data || []; | ||
|
|
||
| if (cohortIds.length === 0) { | ||
| return { devices: [], meta: {} }; | ||
| } | ||
|
|
||
| // Step 2: Fetch devices for the retrieved cohorts | ||
| return getDevicesForCohortsApi( | ||
| { | ||
| cohort_ids: cohortIds, | ||
| skip: params.skip, | ||
| limit: params.limit, | ||
| category: category, | ||
| ...(search && { search }), | ||
| signal | ||
| }, | ||
| ); | ||
| } catch (error) { | ||
| if (error.name === 'AbortError' || error.code === 'ECONNABORTED') { | ||
| throw new Error('Request cancelled'); | ||
| } | ||
| // Handle cohort fetch errors | ||
| throw new Error( | ||
| `Unable to fetch devices: ${getApiErrorMessage(error)}` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const { getDevicesSummaryApi } = await import('../apis/Analytics'); // Or wherever your all-devices API is | ||
| return getDevicesSummaryApi({ | ||
| ...params, | ||
| category, | ||
| ...(search && { search }), | ||
| signal | ||
| }); | ||
|
|
||
| }, | ||
| [normalizedGroup], | ||
| [normalizedGroup, category, search], |
There was a problem hiding this comment.
🧩 Analysis chain
Same fixes for devices: preserve aborts, robust cohort IDs, zeroed meta; plus verify Analytics API supports category.
- Don’t wrap aborts; rethrow so the outer fetcher swallows them.
- Extract cohort IDs defensively.
- Return explicit zero meta when empty.
- Signal is correctly passed in options to
getDevicesForCohortsApinow. Good.
Apply this diff:
export const usePaginatedDevicesSummary = (group, options = {}) => {
const { search = '', category = 'lowcost', ...rest } = options;
const normalizedGroup =
typeof group === 'string' && group.trim().length > 0 ? group.trim() : '';
const fetcher = useCallback(
async (params, signal) => {
if (normalizedGroup) {
const { getGroupCohortsApi } = await import('../apis/Account');
const { getDevicesForCohortsApi } = await import('../apis/DeviceRegistry');
// Step 1: Fetch cohorts for the group
try {
- const cohortsResponse = await getGroupCohortsApi(normalizedGroup, signal);
- const cohortIds = cohortsResponse?.data || [];
+ const cohortsResponse = await getGroupCohortsApi(normalizedGroup, signal);
+ const raw = cohortsResponse;
+ const source =
+ Array.isArray(raw)
+ ? raw
+ : Array.isArray(raw?.data)
+ ? raw.data
+ : raw?.cohorts ?? raw?.results ?? [];
+ const cohortIds = (Array.isArray(source) ? source : [])
+ .map((c) => (typeof c === 'string' ? c : c?._id ?? c?.id))
+ .filter(Boolean);
if (cohortIds.length === 0) {
- return { devices: [], meta: {} };
+ return {
+ devices: [],
+ meta: { total: 0, totalPages: 0, nextPage: null, previousPage: null },
+ };
}
// Step 2: Fetch devices for the retrieved cohorts
return getDevicesForCohortsApi(
{
cohort_ids: cohortIds,
skip: params.skip,
limit: params.limit,
- category: category,
+ category,
...(search && { search }),
- signal
+ signal
},
);
} catch (error) {
- if (error.name === 'AbortError' || error.code === 'ECONNABORTED') {
- throw new Error('Request cancelled');
- }
+ if (
+ error?.name === 'AbortError' ||
+ error?.name === 'CanceledError' ||
+ error?.code === 'ERR_CANCELED' ||
+ error?.code === 'ECONNABORTED'
+ ) {
+ throw error;
+ }
// Handle cohort fetch errors
throw new Error(
`Unable to fetch devices: ${getApiErrorMessage(error)}`
);
}
}
- const { getDevicesSummaryApi } = await import('../apis/Analytics'); // Or wherever your all-devices API is
+ const { getDevicesSummaryApi } = await import('../apis/Analytics');
return getDevicesSummaryApi({
...params,
category,
...(search && { search }),
signal
});
},
[normalizedGroup, category, search],
);Additionally, please verify that getDevicesSummaryApi exists and accepts a category param in the Analytics API.
Also applies to: 440-446
🏁 Script executed:
#!/bin/bash
# Verify the Analytics devices summary API exists and supports `category`
echo "Locate getDevicesSummaryApi definition:"
rg -n -C3 --type=js --type=ts 'export\s+const\s+getDevicesSummaryApi' src
echo
echo "Inspect parameter handling (look for category in params):"
rg -n -C3 --type=js --type=ts 'getDevicesSummaryApi\s*=\s*async|\{.*category.*\}|DEVICES_SUMMARY_URL' src/platform/src/core/apis
echo
echo "Search for other references to confirm expected shape:"
rg -n -C3 --type=js --type=ts '\bgetDevicesSummaryApi\s*\(' srcLength of output: 3521
🏁 Script executed:
#!/bin/bash
echo "Searching for getDevicesSummaryApi definitions in Analytics API:"
rg -n -C3 'getDevicesSummaryApi' src/platform/src/core/apis/Analytics.js || echo "No matches found"
echo
echo "Looking for any export of getDevicesSummaryApi:"
rg -n -C3 'export\s+function\s+getDevicesSummaryApi' src/platform/src/core/apis/Analytics.js || echo "No explicit function export"
echo
echo "Searching for default export patterns referencing getDevicesSummaryApi:"
rg -n -C3 'export\s+default\s+' src/platform/src/core/apis/Analytics.js || echo "No default export"Length of output: 249
Fix missing Analytics API export and verify signature
getDevicesSummaryApiis not exported fromsrc/platform/src/core/apis/Analytics.js; update the export or correct the import path.- Ensure its implementation accepts a
categoryparameter in its options.
🤖 Prompt for AI Agents
In src/platform/src/core/hooks/usePaginatedData.js around lines 385 to 436, the
code imports getDevicesSummaryApi from ../apis/Analytics but that export is
missing or has a different name/signature; update the Analytics module to export
getDevicesSummaryApi (or change this import to the correct exported name/path)
and ensure the exported function accepts an options object that includes
category, search, skip, limit and signal so the call here works as expected.
There was a problem hiding this comment.
It is the correct path; found in Analytics.js. Check that file to confirm
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Analytics.js file for getDevicesSummaryApi export and signature
echo "=== Examining Analytics.js file ==="
cat src/platform/src/core/apis/Analytics.js | head -100
echo -e "\n=== Searching for getDevicesSummaryApi in Analytics.js ==="
rg -n -A10 'getDevicesSummaryApi' src/platform/src/core/apis/Analytics.js
echo -e "\n=== Checking all exports in Analytics.js ==="
rg -n 'export' src/platform/src/core/apis/Analytics.js | head -50
Length of output: 2891
@Codebmk, you're right that there's a devices function in Analytics.js, but I found two issues:
1. Import name mismatch:
- Line 428 imports:
getDevicesSummaryApi(plural "Devices") - Analytics.js exports:
getDeviceSummaryApi(singular "Device")
2. Missing category parameter:
The actual getDeviceSummaryApi function in Analytics.js (line 62) has this signature:
export const getDeviceSummaryApi = async ({
group = null,
skip = 0,
limit = 80,
search = undefined,
signal,
} = {})It doesn't support the category parameter that's being passed on line 429.
Questions:
- Should the import be corrected to
getDeviceSummaryApi(without the 's')? - Does
getDeviceSummaryApineed to be updated to accept acategoryparameter? - Or is there a different API endpoint that should be used for category-filtered devices?
🧬 Code Graph Analysis Results
[src/platform/src/core/apis/Analytics.js] // lines 31-59
export const getSitesSummaryApi = async ({
group = undefined,
skip = 0,
limit = 80,
search = undefined,
signal,
} = {}) => {
const params = {
skip,
limit,
...(group !== undefined && group !== null && group !== '' ? { group } : {}),
...(search !== undefined && search !== null && search.trim() !== ''
? { search: search.trim() }
: {}),
};
return secureApiProxy
.get(SITES_SUMMARY_URL, {
params,
authType: AUTH_TYPES.JWT,
signal,
})
.then((response) => response.data)
.catch((error) => {
logger.error('getSitesSummaryApi error:', error);
// Re-throw the error so it can be handled by SWR and components
throw error;
});
};
[src/platform/src/core/apis/Analytics.js] // lines 93-120
const getGridSummaryApi = async ({
admin_level = null,
skip = 0,
limit = 80,
search = undefined,
signal,
} = {}) => {
const params = {
skip,
limit,
...(admin_level ? { admin_level } : {}),
...(search !== undefined && search !== null && search.trim() !== ''
? { search: search.trim() }
: {}),
};
return secureApiProxy
.get(GRID_SUMMARY_URL, {
params,
authType: AUTH_TYPES.JWT,
signal,
})
.then((response) => response.data)
.catch((error) => {
logger.error('getGridSummaryApi error:', error);
throw error;
});
};
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Checking import statement in usePaginatedData.js ==="
rg -n -C2 "getDevice.*SummaryApi.*import" src/platform/src/core/hooks/usePaginatedData.js
echo -e "\n=== Checking getDeviceSummaryApi export and signature in Analytics.js ==="
rg -n -A15 "export const getDeviceSummaryApi" src/platform/src/core/apis/Analytics.js
echo -e "\n=== Verifying category parameter usage ==="
rg -n -C3 "category" src/platform/src/core/apis/Analytics.js | grep -A3 -B3 "getDeviceSummaryApi"
Length of output: 776
@Codebmk, perfect! I've confirmed both issues are resolved:
✅ Import name corrected: Line 427 now correctly imports getDeviceSummaryApi (singular "Device") matching the export in Analytics.js
✅ Category parameter added: Line 68 in Analytics.js shows the function signature now includes category = 'lowcost' as a parameter, and it's properly used in the params object on line 72
The implementation looks solid now. Great work! 🎉
🧬 Code Graph Analysis Results
- Filename: src/platform/src/core/apis/DeviceRegistry.js
- Snippet (lines 86-132)
- Content:
const getDevicesForCohortsApi = ({
cohort_ids = [],
skip = 0,
limit = 30,
search='',
category,
signal,
} = {}) => {
// Sanitize pagination parameters
const sanitizedSkip = Math.max(0, parseInt(skip, 10) || 0);
const sanitizedLimit = Math.max(1, Math.min(100, parseInt(limit, 10) || 30));
// Put pagination parameters in query params
const params = {
skip: sanitizedSkip,
limit: sanitizedLimit,
...(category && { category }),
...(search && { search }),
};
// Put cohort_ids in request body
const body = {
cohort_ids: Array.isArray(cohort_ids) ? cohort_ids : [],
};
const config = {
params,
authType: AUTH_TYPES.JWT,
};
// Add abort signal if provided and still valid
if (signal && !signal.aborted) {
config.signal = signal;
}
return secureApiProxy
.post(DEVICES_FOR_COHORTS_URL, body, config)
.then((response) => response.data)
.catch((error) => {
// Handle abort errors gracefully
if (error.name === 'AbortError' || error.code === 'ECONNABORTED') {
throw new Error('Request cancelled');
}
// Re-throw other errors
throw error;
});
};
- Filename: src/platform/src/core/apis/DeviceRegistry.js
- Snippet (lines 86-132)
- Content:
const getDevicesForCohortsApi = ({
cohort_ids = [],
skip = 0,
limit = 30,
search='',
category,
signal,
} = {}) => {
// Sanitize pagination parameters
const sanitizedSkip = Math.max(0, parseInt(skip, 10) || 0);
const sanitizedLimit = Math.max(1, Math.min(100, parseInt(limit, 10) || 30));
// Put pagination parameters in query params
const params = {
skip: sanitizedSkip,
limit: sanitizedLimit,
...(category && { category }),
...(search && { search }),
};
// Put cohort_ids in request body
const body = {
cohort_ids: Array.isArray(cohort_ids) ? cohort_ids : [],
};
const config = {
params,
authType: AUTH_TYPES.JWT,
};
// Add abort signal if provided and still valid
if (signal && !signal.aborted) {
config.signal = signal;
}
return secureApiProxy
.post(DEVICES_FOR_COHORTS_URL, body, config)
.then((response) => response.data)
.catch((error) => {
// Handle abort errors gracefully
if (error.name === 'AbortError' || error.code === 'ECONNABORTED') {
throw new Error('Request cancelled');
}
// Re-throw other errors
throw error;
});
};
- Filename: src/platform/src/core/apis/Analytics.js
- Snippet (lines 31-59)
- Content:
export const getSitesSummaryApi = async ({
group = undefined,
skip = 0,
limit = 80,
search = undefined,
signal,
} = {}) => {
const params = {
skip,
limit,
...(group !== undefined && group !== null && group !== '' ? { group } : {}),
...(search !== undefined && search !== null && search.trim() !== ''
? { search: search.trim() }
: {}),
};
return secureApiProxy
.get(SITES_SUMMARY_URL, {
params,
authType: AUTH_TYPES.JWT,
signal,
})
.then((response) => response.data)
.catch((error) => {
logger.error('getSitesSummaryApi error:', error);
// Re-throw the error so it can be handled by SWR and components
throw error;
});
};
- Filename: src/platform/src/core/apis/Analytics.js
- Snippet (lines 62-92)
- Content:
export const getDeviceSummaryApi = async ({
group = null,
skip = 0,
limit = 80,
search = undefined,
signal,
category = 'lowcost',
} = {}) => {
const params = {
status: 'deployed',
...(category ? { category } : {}),
skip,
limit,
...(group ? { group } : {}),
...(search !== undefined && search !== null && search.trim() !== ''
? { search: search.trim() }
: {}),
};
return secureApiProxy
.get(DEVICE_SUMMARY_URL, {
params,
authType: AUTH_TYPES.JWT,
signal,
})
.then((response) => response.data)
.catch((error) => {
logger.error('getDeviceSummaryApi error:', error);
throw error;
});
};
- Filename: src/platform/src/core/apis/Analytics.js
- Snippet (lines 95-122)
- Content:
export const getGridSummaryApi = async ({
admin_level = null,
skip = 0,
limit = 80,
search = undefined,
signal,
} = {}) => {
const params = {
skip,
limit,
...(admin_level ? { admin_level } : {}),
...(search !== undefined && search !== null && search.trim() !== ''
? { search: search.trim() }
: {}),
};
return secureApiProxy
.get(GRID_SUMMARY_URL, {
params,
authType: AUTH_TYPES.JWT,
signal,
})
.then((response) => response.data)
.catch((error) => {
logger.error('getGridSummaryApi error:', error);
throw error;
});
};
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
New next-platform changes available for preview here |
1 similar comment
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/platform/src/core/hooks/usePaginatedData.js (1)
391-425: Critical: Apply same fixes for abort handling, cohort extraction, and meta structure.The devices fetcher has identical issues to the sites fetcher:
- Lines 417-418: Wrapping aborts breaks SWR's abort detection.
- Line 399: Cohort ID extraction assumes
cohortsResponse?.datais an array of strings.- Line 402: Returning
{ devices: [], meta: {} }lacks explicit pagination fields.Apply this diff:
const fetcher = useCallback( async (params, signal) => { if (normalizedGroup) { - const { getGroupCohortsApi } = await import('../apis/Account'); const { getDevicesForCohortsApi } = await import('../apis/DeviceRegistry'); - // Step 1: Fetch cohorts for the group try { const cohortsResponse = await getGroupCohortsApi(normalizedGroup, signal); - const cohortIds = cohortsResponse?.data || []; + const raw = cohortsResponse; + const source = + Array.isArray(raw) + ? raw + : Array.isArray(raw?.data) + ? raw.data + : raw?.cohorts ?? raw?.results ?? []; + const cohortIds = (Array.isArray(source) ? source : []) + .map((c) => (typeof c === 'string' ? c : c?._id ?? c?.id)) + .filter(Boolean); if (cohortIds.length === 0) { - return { devices: [], meta: {} }; + return { + devices: [], + meta: { total: 0, totalPages: 0, nextPage: null, previousPage: null }, + }; } - // Step 2: Fetch devices for the retrieved cohorts return getDevicesForCohortsApi( { cohort_ids: cohortIds, skip: params.skip, limit: params.limit, - category: category, + category, ...(search && { search }), signal }, ); } catch (error) { - if (error.name === 'AbortError' || error.code === 'ECONNABORTED') { - throw new Error('Request cancelled'); + if ( + error?.name === 'AbortError' || + error?.name === 'CanceledError' || + error?.code === 'ERR_CANCELED' || + error?.code === 'ECONNABORTED' + ) { + throw error; } - // Handle cohort fetch errors throw new Error( `Unable to fetch devices: ${getApiErrorMessage(error)}` ); } } const { getDeviceSummaryApi } = await import('../apis/Analytics'); return getDeviceSummaryApi({ ...params, category, ...(search && { search }), signal }); - }, [normalizedGroup, category, search], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/platform/src/core/apis/Analytics.js(1 hunks)src/platform/src/core/hooks/usePaginatedData.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/platform/src/core/hooks/usePaginatedData.js (3)
src/platform/src/core/apis/Account.js (3)
getGroupCohortsApi(259-269)getGroupCohortsApi(259-269)params(325-325)src/platform/src/core/apis/DeviceRegistry.js (10)
getSitesForCohortsApi(134-172)getSitesForCohortsApi(134-172)params(48-48)params(99-104)params(147-152)params(232-232)params(257-257)params(281-281)getDevicesForCohortsApi(86-132)getDevicesForCohortsApi(86-132)src/platform/src/core/apis/Analytics.js (7)
params(38-45)params(70-79)params(102-109)getSitesSummaryApi(31-59)getSitesSummaryApi(31-59)getDeviceSummaryApi(62-92)getDeviceSummaryApi(62-92)
src/platform/src/core/apis/Analytics.js (1)
src/platform/src/core/apis/DeviceRegistry.js (6)
params(48-48)params(99-104)params(147-152)params(232-232)params(257-257)params(281-281)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-push-deploy-analytics-platform-preview
🔇 Additional comments (1)
src/platform/src/core/apis/Analytics.js (1)
68-72: LGTM! Category parameter properly integrated.The addition of the
categoryparameter with a sensible default ('lowcost') and conditional inclusion in params is well-implemented. This aligns with the broader cohort-based filtering introduced across the PR.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/src/common/features/analytics-overview/hooks/useAnalyticsDownload.js (1)
187-187: AdddeviceCategoryto the dependency array.The
deviceCategoryparameter is used inside the callback (line 139) but is not included in the dependency array. This could lead to stale closures where the callback uses an outdateddeviceCategoryvalue.Apply this diff:
}, - [chartData, fetchData], + [chartData, fetchData, deviceCategory], );
♻️ Duplicate comments (2)
src/platform/src/core/hooks/usePaginatedData.js (2)
320-364: Critical: Fix abort handling, cohort extraction, meta structure, and parameter leakage.Four issues need to be addressed in the sites fetcher:
Lines 347-348: Wrapping abort errors in
new Error('Request cancelled')breaks SWR's abort detection. The outerpaginatedFetcher(lines 76-84) expects native abort errors to returnundefinedwithout updating the cache. This causes spurious errors during rapid navigation or typing.Line 333: Extracting cohort IDs as
cohortsResponse?.data || []assumes a specific response shape. If the API returns{ cohorts: [...] },{ results: [...] }, or an array of objects with_id/idfields, extraction fails silently.Line 336: Returning
{ sites: [], meta: {} }when no cohorts are found provides an incomplete pagination structure. Components expectingmeta.total,meta.totalPages, etc., may encounter undefined errors.Line 342: The
paramsobject may contain apageproperty that should not be passed to DeviceRegistry APIs.Apply this diff:
const fetcher = useCallback( async (params, signal) => { if (normalizedGroup) { const { getGroupCohortsApi } = await import('../apis/Account'); const { getSitesForCohortsApi } = await import( '../apis/DeviceRegistry' ); try { const cohortsResponse = await getGroupCohortsApi( normalizedGroup, signal, ); - const cohortIds = cohortsResponse?.data || []; + // Extract cohort IDs robustly from various response shapes + const raw = cohortsResponse; + const source = + Array.isArray(raw) + ? raw + : Array.isArray(raw?.data) + ? raw.data + : raw?.cohorts ?? raw?.results ?? []; + const cohortIds = (Array.isArray(source) ? source : []) + .map((c) => (typeof c === 'string' ? c : c?._id ?? c?.id)) + .filter(Boolean); if (cohortIds.length === 0) { - return { sites: [], meta: {} }; + return { + sites: [], + meta: { total: 0, totalPages: 0, nextPage: null, previousPage: null }, + }; } + const { page: _page, ...restParams } = params; return getSitesForCohortsApi({ cohort_ids: cohortIds, - ...params, + ...restParams, ...(search && { search }), signal, }); } catch (error) { - if (error.name === 'AbortError' || error.code === 'ECONNABORTED') { - throw new Error('Request cancelled'); + // Preserve abort identity so SWR can handle gracefully + if ( + error?.name === 'AbortError' || + error?.name === 'CanceledError' || + error?.code === 'ERR_CANCELED' || + error?.code === 'ECONNABORTED' + ) { + throw error; } throw new Error( `Unable to fetch sites: ${getApiErrorMessage(error)}` ); } } const { getSitesSummaryApi } = await import('../apis/Analytics'); return getSitesSummaryApi({ ...params, ...(search && { search }), signal }); }, [normalizedGroup, search], );
389-437: Critical: Apply the same fixes to the devices fetcher.The devices fetcher has the same issues as the sites fetcher:
- Lines 417-418: Wrapping abort errors breaks SWR's abort detection.
- Line 399: Fragile cohort ID extraction.
- Line 402: Incomplete meta structure.
Additionally, verify that
paramsdoesn't leak unwanted properties to the API calls.Apply this diff:
const fetcher = useCallback( async (params, signal) => { if (normalizedGroup) { const { getGroupCohortsApi } = await import('../apis/Account'); const { getDevicesForCohortsApi } = await import('../apis/DeviceRegistry'); try { const cohortsResponse = await getGroupCohortsApi(normalizedGroup, signal); - const cohortIds = cohortsResponse?.data || []; + // Extract cohort IDs robustly from various response shapes + const raw = cohortsResponse; + const source = + Array.isArray(raw) + ? raw + : Array.isArray(raw?.data) + ? raw.data + : raw?.cohorts ?? raw?.results ?? []; + const cohortIds = (Array.isArray(source) ? source : []) + .map((c) => (typeof c === 'string' ? c : c?._id ?? c?.id)) + .filter(Boolean); if (cohortIds.length === 0) { - return { devices: [], meta: {} }; + return { + devices: [], + meta: { total: 0, totalPages: 0, nextPage: null, previousPage: null }, + }; } return getDevicesForCohortsApi( { cohort_ids: cohortIds, skip: params.skip, limit: params.limit, ...(category && { category }), ...(search && { search }), signal }, ); } catch (error) { - if (error.name === 'AbortError' || error.code === 'ECONNABORTED') { - throw new Error('Request cancelled'); + if ( + error?.name === 'AbortError' || + error?.name === 'CanceledError' || + error?.code === 'ERR_CANCELED' || + error?.code === 'ECONNABORTED' + ) { + throw error; } throw new Error( `Unable to fetch devices: ${getApiErrorMessage(error)}` ); } } const { getDeviceSummaryApi } = await import('../apis/Analytics'); return getDeviceSummaryApi({ ...params, ...(category && { category }), ...(search && { search }), signal }); }, [normalizedGroup, category, search], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/platform/src/common/features/analytics-overview/hooks/useAnalyticsDownload.js(2 hunks)src/platform/src/common/features/download-insights-locations/data-download/components/DataContent.js(3 hunks)src/platform/src/common/features/download-insights-locations/more-insights/hooks/useDownload.js(3 hunks)src/platform/src/core/hooks/usePaginatedData.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/platform/src/common/features/download-insights-locations/data-download/components/DataContent.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/platform/src/core/hooks/usePaginatedData.js (3)
src/platform/src/core/apis/Account.js (3)
getGroupCohortsApi(259-269)getGroupCohortsApi(259-269)params(325-325)src/platform/src/core/apis/DeviceRegistry.js (10)
getSitesForCohortsApi(134-172)getSitesForCohortsApi(134-172)params(48-48)params(99-104)params(147-152)params(232-232)params(257-257)params(281-281)getDevicesForCohortsApi(86-132)getDevicesForCohortsApi(86-132)src/platform/src/core/apis/Analytics.js (7)
params(38-45)params(70-79)params(102-109)getSitesSummaryApi(31-59)getSitesSummaryApi(31-59)getDeviceSummaryApi(62-92)getDeviceSummaryApi(62-92)
src/platform/src/common/features/download-insights-locations/more-insights/hooks/useDownload.js (2)
src/platform/src/common/features/download-insights-locations/more-insights/hooks/useMoreInsights.js (4)
visibleSites(93-93)allSites(22-28)dateRange(90-90)frequency(74-74)src/platform/src/common/features/airQuality-map/hooks/useMapPageState.js (1)
pollutant(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-push-deploy-analytics-platform-preview
🔇 Additional comments (4)
src/platform/src/common/features/download-insights-locations/more-insights/hooks/useDownload.js (1)
84-92: LGTM: Dependency array correctly includesdeviceCategory.The addition of
deviceCategoryto the dependency array is correct and ensures the callback is properly re-created when the category changes.src/platform/src/common/features/analytics-overview/hooks/useAnalyticsDownload.js (1)
79-83: LGTM: Clean function signature update.The addition of
deviceCategoryparameter with a default value is well-implemented and follows a cleaner pattern compared to thearguments[0]approach used elsewhere.src/platform/src/core/hooks/usePaginatedData.js (2)
440-440: LGTM: SWR cache key properly includescategory.The cache key correctly includes the
categoryparameter, ensuring that data is properly cached and invalidated when the category changes.
5-5: LGTM: Good addition for consistent error handling.The
getApiErrorMessageutility import supports consistent error messaging across the cohort-based fetching flows.
src/platform/src/common/features/download-insights-locations/more-insights/hooks/useDownload.js
Outdated
Show resolved
Hide resolved
…eta; avoid leaking page into DeviceRegistry calls.
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/platform/src/core/hooks/usePaginatedData.js (2)
366-371: Consider strippingpagefrom params.The
...paramsspread at line 368 may include apageproperty, whichgetSitesForCohortsApidoesn't use (it expectsskip/limit). While not harmful, this was flagged in earlier reviews. For cleaner API calls, consider destructuring to excludepage:// Step 2: Fetch sites for the retrieved cohorts + const { page: _page, ...restParams } = params; return getSitesForCohortsApi({ cohort_ids: cohortIds, - ...params, + ...restParams, ...(search && { search }), signal, });
373-373: Standardize abort error checks.Lines 373 and 441 check only
error.name === 'AbortError'anderror.code === 'ECONNABORTED'. However, the mainpaginatedFetcher(lines 76-80) also checks for'CanceledError'and'ERR_CANCELED', which Axios throws when usingaxios.CancelTokenorsignal. For consistency and completeness, consider aligning these checks.Apply this diff to both locations:
} catch (error) { - if (error.name === 'AbortError' || error.code === 'ECONNABORTED') throw error; + if ( + error?.name === 'AbortError' || + error?.name === 'CanceledError' || + error?.code === 'ERR_CANCELED' || + error?.code === 'ECONNABORTED' + ) { + throw error; + } // Handle cohort fetch errors throw new Error(Also applies to: 441-441
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/platform/src/core/hooks/usePaginatedData.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/platform/src/core/hooks/usePaginatedData.js (3)
src/platform/src/core/apis/Analytics.js (9)
response(16-18)response(24-26)params(38-45)params(70-79)params(102-109)getSitesSummaryApi(31-59)getSitesSummaryApi(31-59)getDeviceSummaryApi(62-92)getDeviceSummaryApi(62-92)src/platform/src/core/apis/Account.js (3)
getGroupCohortsApi(259-269)getGroupCohortsApi(259-269)params(325-325)src/platform/src/core/apis/DeviceRegistry.js (10)
params(48-48)params(99-104)params(147-152)params(232-232)params(257-257)params(281-281)getSitesForCohortsApi(134-172)getSitesForCohortsApi(134-172)getDevicesForCohortsApi(86-132)getDevicesForCohortsApi(86-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-push-deploy-analytics-platform-preview
- GitHub Check: test-analytics-platform-changes
🔇 Additional comments (2)
src/platform/src/core/hooks/usePaginatedData.js (2)
310-335: Solid defensive extraction of cohort IDs.The
extractCohortIdshelper robustly handles multiple API response formats (direct array,data/cohorts/resultswrappers, object vs. string IDs) and gracefully returns an empty array for malformed responses. This addresses the earlier concern about fragile cohort ID extraction.
430-439: Good explicit parameter passing.Unlike the sites fetcher, here you explicitly pass
skip,limit,category, andsearchinstead of spreadingparams. This avoids the potential issue of passing unnecessary properties likepageto the API.
|
New next-platform changes available for preview here |
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/platform/src/core/hooks/usePaginatedData.js (1)
344-391: Solid cohort-based implementation.The refactored sites fetcher correctly:
- Uses
extractCohortIdsfor defensive cohort extraction- Preserves abort error identity (line 373) without wrapping
- Returns properly structured meta with
limitfallback when empty (line 362)- Applies
getApiErrorMessagefor consistent error formattingMinor observation: The sites hook spreads all
params(line 368) while the devices hook explicitly passes onlyskip,limit, etc. (lines 433-434). Both work correctly, but explicit field selection is slightly cleaner and more self-documenting.Optionally consider passing only necessary fields for consistency:
- return getSitesForCohortsApi({ - cohort_ids: cohortIds, - ...params, - ...(search && { search }), - signal, - }); + return getSitesForCohortsApi({ + cohort_ids: cohortIds, + skip: params.skip, + limit: params.limit, + ...(search && { search }), + signal, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/platform/src/common/features/download-insights-locations/more-insights/hooks/useDownload.js(3 hunks)src/platform/src/core/hooks/usePaginatedData.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/platform/src/common/features/download-insights-locations/more-insights/hooks/useDownload.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/platform/src/core/hooks/usePaginatedData.js (3)
src/platform/src/core/apis/Analytics.js (9)
response(16-18)response(24-26)params(38-45)params(70-79)params(102-109)getSitesSummaryApi(31-59)getSitesSummaryApi(31-59)getDeviceSummaryApi(62-92)getDeviceSummaryApi(62-92)src/platform/src/core/apis/Account.js (3)
getGroupCohortsApi(259-269)getGroupCohortsApi(259-269)params(325-325)src/platform/src/core/apis/DeviceRegistry.js (10)
params(48-48)params(99-104)params(147-152)params(232-232)params(257-257)params(281-281)getSitesForCohortsApi(134-172)getSitesForCohortsApi(134-172)getDevicesForCohortsApi(86-132)getDevicesForCohortsApi(86-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-push-deploy-analytics-platform-preview
🔇 Additional comments (3)
src/platform/src/core/hooks/usePaginatedData.js (3)
5-5: Good addition for consistent error messaging.The
getApiErrorMessageimport supports centralized error formatting in the new cohort-based flows.
310-335: Well-implemented defensive extraction.The
extractCohortIdshelper elegantly handles multiple API response shapes (nested arrays, direct arrays, objects with_id/id, string arrays) and provides a safe empty array fallback. Good defensive programming.
408-471: Excellent devices hook implementation.The devices fetcher correctly:
- Handles
categoryparameter throughout cohort and non-cohort flows- Explicitly passes only necessary fields to
getDevicesForCohortsApi(lines 433-434) for clarity- Preserves abort error identity (line 441)
- Includes comprehensive
useCallbackdependencies (line 458) and SWR key (line 462)The implementation is clean, consistent, and well-structured.
2 participants
Summary by CodeRabbit
New Features
Improvements
Refactor
Reliability
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)