Enhance analytics components with error handling and loading states - #3522
Merged
Conversation
…update hooks for improved data fetching
…date analytics components for improved error handling
…nhance subscription management with mounted state checks; update chart components for better responsiveness and error handling; streamline user actions and service responses.
…; introduce getSiteDisplayName utility for consistent site name resolution
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR centralizes site/location display-name resolution, refactors analytics site-card construction to use chart points, enhances QuickAccess error/loading UX, adds async abort/mounted guards in billing and polling flows, integrates Paddle (provider, types, layout wiring), and polishes chart/layout behavior. ChangesSite Naming Normalization and Analytics Refactoring
🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
|
New azure analytics_platform changes available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/platform/src/shared/services/subscriptionService.ts (1)
753-765:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback usage can return null limits during live-usage failures.
On Line 764, fallback uses only
profile?.apiRateLimits. If that optional payload is absent, the catch path (Lines 796-803) returns usage limits asnulldespite the message saying plan limits are being shown.Suggested fix
- const getFallbackRateLimits = async (): Promise< - ApiRateLimitsPayload | undefined - > => { + const getFallbackRateLimits = async (): Promise<ApiRateLimitsPayload> => { if (!profile) { try { profile = await this.getUsersProfilePayload(); } catch { profile = null; } } - return profile?.apiRateLimits ?? undefined; + return mergeRateLimitsWithDefaults( + normalizeTier(profile?.subscriptionTier), + profile?.apiRateLimits + ); };Also applies to: 778-783, 796-803
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/src/shared/services/subscriptionService.ts` around lines 753 - 765, getFallbackRateLimits currently returns profile?.apiRateLimits which can be null, causing "plan limits" to actually be null during failures; change the flow so when profile exists but apiRateLimits is missing (or profile fetch fails) you return a non-null safe ApiRateLimitsPayload (either derive sensible limits from profile.plan if available or fall back to a new/shared DEFAULT_API_RATE_LIMITS constant). Update getFallbackRateLimits to: attempt this.getUsersProfilePayload(), then if profile.apiRateLimits is falsy build and return a default ApiRateLimitsPayload (do not return null); apply the same fix to the other similar fallback blocks referenced (the blocks around the other getUsersProfilePayload usages) so all branches return a concrete ApiRateLimitsPayload instead of null.src/platform/src/modules/billing/components/SubscriptionSection.tsx (1)
86-129: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftPrefer SWR or React Query over manual fetch-and-guard pattern.
While the
isMountedRefguard correctly prevents state updates after unmount, this manual fetch pattern violates the guideline to use SWR/React Query. SincerefreshDatais invoked both on mount and after user actions (enable/disable auto-renew, cancellation), using SWR with mutation would provide automatic revalidation, request deduplication, and built-in cancellation.Consider migrating to SWR:
import useSWR from 'swr'; const { data: subscription, mutate: refreshSubscription } = useSWR( 'user-subscription', () => subscriptionService.getSubscription() ); const { data: plans } = useSWR( 'subscription-plans', () => subscriptionService.getPlans() ); // After actions: await refreshSubscription();This would eliminate manual mount guards, simplify the async flow, and provide automatic cancellation via SWR's internal AbortController. As per coding guidelines, client fetches should prefer SWR/React Query with stable keys instead of custom fetch loops.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/src/modules/billing/components/SubscriptionSection.tsx` around lines 86 - 129, The current manual fetch pattern in refreshData (used with isMountedRef, setSubscription, setPlans and invoked in useEffect) should be replaced with SWR/React Query: create useSWR hooks for subscriptionService.getSubscription and subscriptionService.getPlans (e.g., useSWR('user-subscription', () => subscriptionService.getSubscription()) and useSWR('subscription-plans', () => subscriptionService.getPlans())), remove the isMountedRef guard and the refreshData function and its useEffect, and wire post-action refreshes (enable/disable auto-renew, cancellation) to the returned mutate or refresh functions from useSWR to revalidate; ensure error handling and loading state use SWR’s error and isLoading values instead of manual try/catch and setLoading.
🧹 Nitpick comments (2)
src/platform/src/modules/location-insights/more-insights.tsx (2)
202-273: ⚡ Quick winConsider adding user-facing error state for failed chart loads.
Currently, when chart data fetching fails, the error is only logged to console and the chart shows empty data. Users have no indication that a fetch error occurred versus legitimately empty data.
💡 Example error state handling
Add error state:
const [chartData, setChartData] = useState<ChartData[]>([]); +const [chartError, setChartError] = useState<string | null>(null);Update the effect:
setChartData([]); + setChartError(null); const run = async () => { try { // ... existing fetch logic } catch (error) { if (!isActive) { return; } + if (error?.name !== 'AbortError' && error?.name !== 'CanceledError') { + setChartError('Failed to load chart data. Please try again.'); + } console.error('Error fetching chart data:', error); setChartData([]); } };Pass error to ChartContainer:
<ChartContainer title={...} loading={isChartLoading} + error={chartError} className="h-full flex flex-col border"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/src/modules/location-insights/more-insights.tsx` around lines 202 - 273, Add a user-facing error state: introduce an error state (e.g., chartError) alongside setChartData and clear it whenever you start a new fetch or when the dialog closes; in the useEffect surrounding getChartData (the async run function), set chartError to null before calling getChartData, set chartError to the caught error message inside the catch block (only if isActive), and ensure you also clear chartError when you setChartData([]) for legitimately empty responses; finally pass chartError into the ChartContainer (or whatever component consumes chart data) so it can render an error UI instead of an empty chart—refer to useEffect, run, getChartData, normalizeAirQualityData, setChartData, and isActive to locate where to update state and where to pass the new prop.
202-273: ⚡ Quick winAdd AbortController to cancel in-flight chart requests.
The chart data fetch can be re-fired when dependencies change (filters, date range, visible sites), but uses an
isActiveflag instead of AbortController. While the flag prevents stale state updates, it doesn't cancel the in-flight HTTP request, wasting resources and potentially causing issues with rapid filter changes.As per coding guidelines, "Use AbortController for cohort endpoints and any query that can be re-fired on re-render."
♻️ Refactor to add request cancellation
useEffect(() => { let isActive = true; + const abortController = new AbortController(); if ( !isOpen || visibleSiteIds.length === 0 || !dateRange?.from || !dateRange?.to ) { setChartData([]); return () => { isActive = false; + abortController.abort(); }; } setChartData([]); const run = async () => { try { const response = await getChartData({ sites: visibleSiteIds, startDate: dateRange.from.toISOString().split('T')[0], endDate: dateRange.to.toISOString().split('T')[0], chartType: chartType, frequency: frequency, pollutant: pollutant.toLowerCase().replace('.', '_'), organisation_name: '', + signal: abortController.signal, }); if (!isActive) { return; } if ( response?.data && Array.isArray(response.data) && response.data.length > 0 ) { const transformed = normalizeAirQualityData( response.data as ChartDataPoint[] ); setChartData(transformed); return; } setChartData([]); } catch (error) { if (!isActive) { return; } + // Don't log cancellation as error + if (error?.name === 'AbortError' || error?.name === 'CanceledError') { + return; + } console.error('Error fetching chart data:', error); setChartData([]); } }; run(); return () => { isActive = false; + abortController.abort(); }; }, [Note: Verify that
getChartDataaccepts asignalparameter. If the underlying implementation doesn't support it, you may need to update the hook.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/src/modules/location-insights/more-insights.tsx` around lines 202 - 273, The effect currently uses an isActive flag to avoid updating state from stale responses but doesn't cancel the in-flight request; update the useEffect to create an AbortController, pass its signal into getChartData (verify getChartData accepts a signal or update it accordingly), and call controller.abort() in the cleanup; inside the run try/catch handle aborts by checking error.name === 'AbortError' (or early-return on signal.aborted) so you avoid setChartData after abort, and keep existing isActive logic only if needed for extra safety (references: useEffect, run, getChartData, setChartData, isActive).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/platform/src/modules/analytics/components/QuickAccessCard.tsx`:
- Around line 31-35: The empty/error state logic hides usable site cards when an
error occurs and prints errorMessage verbatim; update the conditions so the
empty/error UI is only shown when sites.length === 0 (e.g., compute
shouldShowErrorState as !isLoading && sites.length === 0 &&
Boolean(errorMessage)) and adjust shouldShowAddFavorite/shouldShowSkeleton
accordingly, and sanitize/map errorMessage before rendering (handle cancellation
errors by treating them as non-user-facing, strip or replace tokens/session
data, and map backend/network/internal error texts to a user-friendly message
variable like displayErrorMessage used in the rendering code).
In `@src/platform/src/modules/analytics/hooks/index.ts`:
- Around line 111-130: The card currently computes percentageDifference and
trend from the immediately previous 24h point (using latestValue and
previousValue) while the UI copy describes a "last week" comparison; update this
code to compute week-over-week delta instead: locate where percentageDifference
and trend are produced (symbols: percentageDifference, generateTrend,
latestValue, previousValue, selectedSite) and replace previousValue with a value
sampled from ~7 days earlier (previousWeekValue) when available, compute
percentageDifference = Number.isFinite(previousWeekValue) && previousWeekValue
!== 0 ? ((latestValue - previousWeekValue)/Math.abs(previousWeekValue))*100 : 0
and call generateTrend(latestValue, previousWeekValue); preserve the existing
fallback to 0/trend-neutral if a week-ago value is missing so behavior remains
safe.
In `@src/platform/src/modules/billing/components/SubscriptionManagement.tsx`:
- Around line 19-66: Replace the custom useEffect/fetchSubscription pattern with
SWR: remove the isMounted guard and the fetchSubscription async function, call
useSWR with a stable key (e.g., 'user-subscription') and a fetcher that invokes
subscriptionService.getSubscription(), map the returned data to the same
normalizedSubscription shape (preserve the automaticRenewal/autoRenewal logic)
and update local state via setSubscription; use SWR's isLoading/error instead of
setLoading and the try/catch/toast block (or show toast on error when error from
useSWR exists). Ensure you import useSWR and keep references to
subscriptionService.getSubscription, setSubscription, and the normalization
logic so behavior remains identical.
In `@src/platform/src/modules/billing/components/UsageStats.tsx`:
- Around line 31-69: Replace the manual polling useEffect/fetchUsage/interval
pattern with SWR: remove the useEffect, isActive flag, fetchUsage function, and
clearInterval logic and instead call useSWR with a stable key (e.g.,
'api-usage-stats') and a fetcher that calls subscriptionService.getUsage(), set
refreshInterval to 5*60*1000 and revalidateOnFocus per your preference; then map
the returned data/error/isLoading from useSWR to your existing state handlers
(setUsage, setLiveUsage, setUsageMessage, setLoading) or derive UI state
directly from the SWR response so you no longer manually manage polling, aborts,
or deduplication.
- Around line 31-69: The polling logic in the UsageStats component creates
overlapping requests because fetchUsage has no cancellation; modify fetchUsage
to use an AbortController (create a new controller each invocation), pass its
signal into subscriptionService.getUsage (e.g., getUsage({ signal })) and store
the current controller in a ref so you can abort the previous controller before
starting a new fetch; also abort the active controller in the cleanup return and
ignore AbortError in the catch block (only log non-abort errors) and ensure
setLoading(false) still runs only when not aborted.
---
Outside diff comments:
In `@src/platform/src/modules/billing/components/SubscriptionSection.tsx`:
- Around line 86-129: The current manual fetch pattern in refreshData (used with
isMountedRef, setSubscription, setPlans and invoked in useEffect) should be
replaced with SWR/React Query: create useSWR hooks for
subscriptionService.getSubscription and subscriptionService.getPlans (e.g.,
useSWR('user-subscription', () => subscriptionService.getSubscription()) and
useSWR('subscription-plans', () => subscriptionService.getPlans())), remove the
isMountedRef guard and the refreshData function and its useEffect, and wire
post-action refreshes (enable/disable auto-renew, cancellation) to the returned
mutate or refresh functions from useSWR to revalidate; ensure error handling and
loading state use SWR’s error and isLoading values instead of manual try/catch
and setLoading.
In `@src/platform/src/shared/services/subscriptionService.ts`:
- Around line 753-765: getFallbackRateLimits currently returns
profile?.apiRateLimits which can be null, causing "plan limits" to actually be
null during failures; change the flow so when profile exists but apiRateLimits
is missing (or profile fetch fails) you return a non-null safe
ApiRateLimitsPayload (either derive sensible limits from profile.plan if
available or fall back to a new/shared DEFAULT_API_RATE_LIMITS constant). Update
getFallbackRateLimits to: attempt this.getUsersProfilePayload(), then if
profile.apiRateLimits is falsy build and return a default ApiRateLimitsPayload
(do not return null); apply the same fix to the other similar fallback blocks
referenced (the blocks around the other getUsersProfilePayload usages) so all
branches return a concrete ApiRateLimitsPayload instead of null.
---
Nitpick comments:
In `@src/platform/src/modules/location-insights/more-insights.tsx`:
- Around line 202-273: Add a user-facing error state: introduce an error state
(e.g., chartError) alongside setChartData and clear it whenever you start a new
fetch or when the dialog closes; in the useEffect surrounding getChartData (the
async run function), set chartError to null before calling getChartData, set
chartError to the caught error message inside the catch block (only if
isActive), and ensure you also clear chartError when you setChartData([]) for
legitimately empty responses; finally pass chartError into the ChartContainer
(or whatever component consumes chart data) so it can render an error UI instead
of an empty chart—refer to useEffect, run, getChartData,
normalizeAirQualityData, setChartData, and isActive to locate where to update
state and where to pass the new prop.
- Around line 202-273: The effect currently uses an isActive flag to avoid
updating state from stale responses but doesn't cancel the in-flight request;
update the useEffect to create an AbortController, pass its signal into
getChartData (verify getChartData accepts a signal or update it accordingly),
and call controller.abort() in the cleanup; inside the run try/catch handle
aborts by checking error.name === 'AbortError' (or early-return on
signal.aborted) so you avoid setChartData after abort, and keep existing
isActive logic only if needed for extra safety (references: useEffect, run,
getChartData, setChartData, isActive).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29d5dec1-bf2a-47d8-b493-a8e3213a65c8
📒 Files selected for processing (27)
src/platform/src/app/(dashboard)/(individual)/user/(auth)/login/page.tsxsrc/platform/src/app/(dashboard)/(organization)/org/[org_slug]/(auth)/login/page.tsxsrc/platform/src/modules/analytics/components/AnalyticsCard.tsxsrc/platform/src/modules/analytics/components/AnalyticsDashboard.tsxsrc/platform/src/modules/analytics/components/DownloadDialog.tsxsrc/platform/src/modules/analytics/components/QuickAccessCard.tsxsrc/platform/src/modules/analytics/hooks/index.tssrc/platform/src/modules/analytics/types/index.tssrc/platform/src/modules/analytics/utils/index.tssrc/platform/src/modules/billing/components/SubscriptionManagement.tsxsrc/platform/src/modules/billing/components/SubscriptionSection.tsxsrc/platform/src/modules/billing/components/UsageStats.tsxsrc/platform/src/modules/location-insights/add-favorites.tsxsrc/platform/src/modules/location-insights/add-location.tsxsrc/platform/src/modules/location-insights/more-insights.tsxsrc/platform/src/shared/components/auth/SelectedEmailCard.tsxsrc/platform/src/shared/components/charts/components/ChartContainer.tsxsrc/platform/src/shared/components/charts/components/charts/DynamicChart.tsxsrc/platform/src/shared/components/charts/components/ui/CustomTooltip.tsxsrc/platform/src/shared/components/charts/types/index.tssrc/platform/src/shared/components/charts/utils/index.tssrc/platform/src/shared/components/ui/wide-dialog.tsxsrc/platform/src/shared/hooks/usePreferences.tssrc/platform/src/shared/hooks/useUserActions.tssrc/platform/src/shared/services/deviceService.tssrc/platform/src/shared/services/subscriptionService.tssrc/platform/src/shared/utils/siteUtils.ts
… abort error detection and improve user feedback for loading states
Contributor
|
New azure analytics_platform changes available for preview here |
…aging for user identification
…ove obsolete script for standalone asset management
Contributor
|
New azure analytics_platform changes available for preview here |
Contributor
There was a problem hiding this comment.
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/modules/analytics/hooks/index.ts (1)
51-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMinor inconsistency in
percentageDifferencedefault values.
buildNoValueSiteCard(line 51) setspercentageDifference: 0, whilebuildSiteCardsFromChartPoints(line 114) returnsundefinedwhen no valid previous value exists. This inconsistency means "no value" cards will show "worsened by 0.0%" in the tooltip, whereas cards with a single reading will show the generic trend message.Consider aligning both to
undefinedfor consistent UX:Suggested fix
const buildNoValueSiteCard = ( selectedSite: Site, pollutant: string ): SiteData => ({ _id: selectedSite._id, name: getSiteDisplayName(selectedSite), location: selectedSite.country || 'Unknown Country', value: 0, status: 'no-value', pollutant, unit: 'μg/m³', trend: 'stable', - percentageDifference: 0, + percentageDifference: undefined, });Also applies to: 111-114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/src/modules/analytics/hooks/index.ts` at line 51, buildNoValueSiteCard currently sets percentageDifference: 0 which conflicts with buildSiteCardsFromChartPoints returning undefined for "no previous value"; change buildNoValueSiteCard to set percentageDifference to undefined (or omit the property) so both paths use undefined for missing comparisons, and if the SiteCard type disallows undefined update the type signature (or make percentageDifference optional) accordingly; update any tooltip/consumer logic that checks for 0 vs undefined to rely on undefined as the "no value" sentinel.
🧹 Nitpick comments (1)
src/platform/src/shared/services/subscriptionService.ts (1)
105-122: 💤 Low valueConsider extracting
isAbortErrorto a shared utility.This helper duplicates the one in
UsageStats.tsx(lines 14-31). Both check the same error signatures (AbortError,CanceledError,ERR_CANCELED,canceled). Extracting to a shared module (e.g.,@/shared/utils/errorUtils.ts) would reduce duplication and ensure consistent abort detection across the codebase.Not blocking, but worth consolidating when convenient.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/src/shared/services/subscriptionService.ts` around lines 105 - 122, The isAbortError helper is duplicated (e.g., in subscriptionService.ts and UsageStats.tsx); extract it into a single shared utility (e.g., export function isAbortError(...) in a new module like errorUtils.ts) and replace the local implementations with imports of that function; ensure the exported utility preserves the same checks (name === 'AbortError'|'CanceledError', code === 'ERR_CANCELED', message === 'canceled'), update imports in SubscriptionService (isAbortError) and UsageStats to use the shared function, and remove the duplicate local definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/platform/src/modules/analytics/hooks/index.ts`:
- Line 51: buildNoValueSiteCard currently sets percentageDifference: 0 which
conflicts with buildSiteCardsFromChartPoints returning undefined for "no
previous value"; change buildNoValueSiteCard to set percentageDifference to
undefined (or omit the property) so both paths use undefined for missing
comparisons, and if the SiteCard type disallows undefined update the type
signature (or make percentageDifference optional) accordingly; update any
tooltip/consumer logic that checks for 0 vs undefined to rely on undefined as
the "no value" sentinel.
---
Nitpick comments:
In `@src/platform/src/shared/services/subscriptionService.ts`:
- Around line 105-122: The isAbortError helper is duplicated (e.g., in
subscriptionService.ts and UsageStats.tsx); extract it into a single shared
utility (e.g., export function isAbortError(...) in a new module like
errorUtils.ts) and replace the local implementations with imports of that
function; ensure the exported utility preserves the same checks (name ===
'AbortError'|'CanceledError', code === 'ERR_CANCELED', message === 'canceled'),
update imports in SubscriptionService (isAbortError) and UsageStats to use the
shared function, and remove the duplicate local definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a344446a-0f94-470d-b856-483eecf498b9
📒 Files selected for processing (9)
src/platform/Dockerfilesrc/platform/package.jsonsrc/platform/scripts/copy-standalone-assets.mjssrc/platform/src/modules/analytics/components/AnalyticsCard.tsxsrc/platform/src/modules/analytics/components/QuickAccessCard.tsxsrc/platform/src/modules/analytics/hooks/index.tssrc/platform/src/modules/billing/components/SubscriptionSection.tsxsrc/platform/src/modules/billing/components/UsageStats.tsxsrc/platform/src/shared/services/subscriptionService.ts
💤 Files with no reviewable changes (1)
- src/platform/scripts/copy-standalone-assets.mjs
…ent configuration
Contributor
|
New azure analytics_platform changes available for preview here |
This was referenced May 30, 2026
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status of maturity (all need to be checked before merging):
Screenshots (optional)
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Chores