Enhance recent readings functionality and PR preview fix#3561
Merged
Conversation
- Implement fetching of recent readings in analytics service with batching - Normalize recent readings data for site cards - Update device service to handle cohort details fallback - Refactor environment variable handling in deployment workflow - Improve login page redirect logic - Add production mode instructions to README - Clean up Dockerfile and package.json for better clarity
Contributor
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR batches and normalizes analytics recent-readings, updates analytics hooks/utilities to prefer recent-readings with chart-data fallback, removes user_id from map readings and types, normalizes auth callback URLs, computes per-PR NEXTAUTH URLs in CI with a runtime providers health check, simplifies build/start and Docker runtime env, tweaks UI/layout components, tightens user-fetch gating, and implements JWT single-flight refresh. ChangesAnalytics Service and Hooks Refactoring
Map Readings, Locations, and Authentication Normalization
CI: Azure preview NEXTAUTH URL and runtime health check
Build and Container Configuration
UI Layout and Components
User Fetching and ApiClient
Sequence Diagram(s)sequenceDiagram
participant Workflow as GitHub Workflow
participant ContainerApp as Azure Container App
participant BuildEnv as Build .env
participant Deploy as Deployment
participant HealthCheck as NextAuth Health Check
Workflow->>ContainerApp: Fetch DEFAULT_DOMAIN for branch app
Workflow->>BuildEnv: Filter auth keys and inject NEXTAUTH_URL/NEXTAUTH_URL_INTERNAL/AUTH_TRUST_HOST
Workflow->>Deploy: Build ENV_VARS array with filtered secrets + computed auth URLs
Workflow->>Deploy: Validate NEXTAUTH_SECRET/AUTH_SECRET present (fail fast)
Deploy->>ContainerApp: az containerapp update --replace-env-vars "${ENV_VARS[@]}"
ContainerApp-->>HealthCheck: container running
HealthCheck->>ContainerApp: GET /api/auth/providers
alt HTTP 200 OK
HealthCheck-->>Workflow: Health check passed
else Not 200
HealthCheck->>Workflow: Print response body & recent container logs
Workflow-->>Workflow: Exit with failure
end
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
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 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 |
OchiengPaul442
changed the title
![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
🧹 Nitpick comments (1)
src/platform/Dockerfile (1)
46-46: ⚡ Quick winDrop the “binds to localhost” critical concern: Next standalone defaults HOSTNAME to
0.0.0.0when unset.
src/platform/DockerfilerunsCMD ["node", "server.js"]withoutENV HOSTNAME, but Next.js standalone mode is documented to listen on0.0.0.0by default whenHOSTNAMEisn’t set—so-p 3000:3000access shouldn’t be broken by this omission. AddingENV HOSTNAME=0.0.0.0would just be for consistency/explicitness.🔧 Proposed fix
USER nextjs EXPOSE 3000 +ENV HOSTNAME=0.0.0.0 CMD ["node", "server.js"]🤖 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/Dockerfile` at line 46, The Dockerfile currently ends with CMD ["node", "server.js"] and omits an explicit HOSTNAME, so add an ENV declaration to make the listening address explicit: insert an ENV HOSTNAME=0.0.0.0 before the CMD to ensure Next.js standalone runs bound to 0.0.0.0 (reference the Dockerfile entry with CMD ["node", "server.js"] and add ENV HOSTNAME=0.0.0.0 above it).
🤖 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 @.github/workflows/deploy-frontend-azurepreview.yml:
- Around line 354-355: The BRANCH/APP values are currently built inside shell
run: steps from attacker-controlled needs.branch-name.outputs.lowercase,
exposing secrets to shell interpretation; move construction out of the run shell
and into GitHub Actions expressions or step/job env so no shell evaluation
occurs (e.g., set env: BRANCH: ${{ needs.branch-name.outputs.lowercase }} and
APP: ${{ format('{0}-analytics-p-preview',
substring(needs.branch-name.outputs.lowercase, 0, 12)) }} or use APP: ${{
concat(substring(needs.branch-name.outputs.lowercase,0,12),
'-analytics-p-preview') }}), and apply the same change to the second run block
and any other preview jobs that use BRANCH/APP; ensure uses of NEXTAUTH_URL and
az containerapp --name reference the env variables (BRANCH/APP) rather than
constructing them inside run so untrusted characters aren’t interpreted by the
shell.
In `@src/platform/package.json`:
- Line 8: The package.json "start" script is wrong for Next.js when
next.config.mjs uses output: 'standalone'; update the start script in
src/platform/package.json so it runs the standalone server (node
.next/standalone/server.js) instead of "next start" to match next.config.mjs and
the README.md "Production Mode" instructions and keep compatibility with the
Dockerfile that runs server.js from .next/standalone.
In `@src/platform/src/modules/analytics/hooks/index.ts`:
- Around line 522-562: The catch block for analyticsService.getRecentReadings
currently unconditionally calls getChartData on any non-abort error; change this
so only a narrowly defined compatibility error triggers the fallback (e.g.,
check error.code or error.response?.status and allow fallback only for the
specific expected compatibility case), and for all other errors (401, 5xx,
ERR_NETWORK, etc.) rethrow the error so the query fails normally; keep the
existing signal.aborted check, and if you need a helper, add an
isCompatibilityFallbackError(error) used before calling
createLatestSiteCardDateRange/getChartData and buildSiteCardsFromChartPoints.
- Around line 484-490: selectedSitesKey currently only includes site._id and
getSiteDisplayName(site) so changes to site.city/region/country (used by
buildSiteCardLocation) won't invalidate caches; update the useMemo that defines
selectedSitesKey to also include the location fields (site.city, site.region,
site.country) for each site so the key reflects those metadata changes and React
Query/SWR will refetch; keep the same selectedSitesKey useMemo and join logic
but append the location values (or a normalized location string) for each site
when mapping selectedSites.
In `@src/platform/src/shared/services/analyticsService.ts`:
- Around line 113-165: Promise.allSettled is hiding aborts; update the
post-allSettled handling in the method that calls
this.serverClient.get/normalizeRecentReadingsResponse so that any rejected
result whose reason is an AbortError (or otherwise indicates signal.aborted)
immediately rethrows that abort reason instead of continuing to merge fulfilled
batches. Concretely: after Promise.allSettled(resolve) check responses for any
rejected entry where the reason is an abort (e.g., reason.name === 'AbortError'
or a helper isAbortError(reason)); if found, throw that reason; otherwise fall
back to the existing logic that aggregates successfulResponses and throws a
non-abort error when no successfulResponses exist.
---
Nitpick comments:
In `@src/platform/Dockerfile`:
- Line 46: The Dockerfile currently ends with CMD ["node", "server.js"] and
omits an explicit HOSTNAME, so add an ENV declaration to make the listening
address explicit: insert an ENV HOSTNAME=0.0.0.0 before the CMD to ensure
Next.js standalone runs bound to 0.0.0.0 (reference the Dockerfile entry with
CMD ["node", "server.js"] and add ENV HOSTNAME=0.0.0.0 above it).
🪄 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: b8d2bfaf-2ef6-4b46-8e16-4db88117fc58
📒 Files selected for processing (13)
.github/workflows/deploy-frontend-azurepreview.ymlsrc/platform/Dockerfilesrc/platform/README.mdsrc/platform/package.jsonsrc/platform/src/app/(dashboard)/(individual)/user/(auth)/login/page.tsxsrc/platform/src/app/(dashboard)/(organization)/org/[org_slug]/(auth)/login/page.tsxsrc/platform/src/app/layout.tsxsrc/platform/src/modules/airqo-map/hooks/useMapReadings.tssrc/platform/src/modules/analytics/hooks/index.tssrc/platform/src/modules/analytics/utils/index.tssrc/platform/src/shared/services/analyticsService.tssrc/platform/src/shared/services/deviceService.tssrc/platform/src/shared/types/api.ts
💤 Files with no reviewable changes (1)
- src/platform/src/shared/types/api.ts
| "prebuild": "yarn clean:next", | ||
| "postbuild": "node -e \"const fs=require('node:fs'); const path=require('node:path'); const projectRoot=process.cwd(); const standaloneRoot=path.join(projectRoot,'.next','standalone'); const publicSource=path.join(projectRoot,'public'); const publicTarget=path.join(standaloneRoot,'public'); const staticSource=path.join(projectRoot,'.next','static'); const staticTarget=path.join(standaloneRoot,'.next','static'); const copyDirectory=(source,target)=>{ if(!fs.existsSync(source)) return false; fs.mkdirSync(path.dirname(target),{recursive:true}); fs.rmSync(target,{recursive:true,force:true}); fs.cpSync(source,target,{recursive:true}); return true; }; if(!fs.existsSync(standaloneRoot)){ throw new Error(`Standalone output not found at ${standaloneRoot}. Run 'yarn build' first.`); } copyDirectory(publicSource, publicTarget); copyDirectory(staticSource, staticTarget);\"", | ||
| "start": "node -e \"const { loadEnvConfig } = require('@next/env'); loadEnvConfig(process.cwd()); process.env.HOSTNAME='localhost'; require('./.next/standalone/server.js')\"", | ||
| "start": "next start", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if next.config uses standalone output mode
rg -n "output.*standalone" src/platform/next.config.mjs src/platform/next.config.jsRepository: airqo-platform/AirQo-frontend
Length of output: 215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm package.json start script
fd -a "package.json" src/platform -g "package.json" 2>/dev/null || true
cat -n src/platform/package.json | sed -n '1,120p'
# 2) Confirm next.config.mjs output mode
cat -n src/platform/next.config.mjs | sed -n '1,120p'
# 3) Locate Dockerfile and inspect relevant parts (standalone/server.js)
fd -a "Dockerfile" . | head -n 20
dockerfile="$(fd -a "Dockerfile" . | head -n 1)"
echo "Using Dockerfile: $dockerfile"
cat -n "$dockerfile" | sed -n '1,220p'Repository: airqo-platform/AirQo-frontend
Length of output: 7279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
dockerfile="src/platform/Dockerfile"
echo "Using Dockerfile: $dockerfile"
test -f "$dockerfile" && echo "found" || { echo "missing"; exit 1; }
cat -n "$dockerfile" | sed -n '1,220p'
# Also double-check what it copies from .next during the runner stage
rg -n "standalone|server\\.js|next start|CMD|COPY --from=builder /app/\\.next" -S "$dockerfile"Repository: airqo-platform/AirQo-frontend
Length of output: 1828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'CMD \[|npm start|next start' src/platform/Dockerfile src/platform/*.Dockerfile 2>/dev/null || trueRepository: airqo-platform/AirQo-frontend
Length of output: 126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "npm run start|next start|server\.js" -S . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/.next/**'Repository: airqo-platform/AirQo-frontend
Length of output: 646
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/platform/README.md | sed -n '1,120p'
cat -n src/platform/README.md | sed -n '120,240p' || true
# Also check if platform Dockerfile comments mention npm start
cat -n src/platform/Dockerfile | sed -n '1,140p'Repository: airqo-platform/AirQo-frontend
Length of output: 3302
🌐 Web query:
Next.js output: 'standalone' can you run next startor must you runnode server.js``
💡 Result:
If your Next.js build uses output: "standalone", you should start the server with node.next/standalone/server.js (the minimal standalone server), not with next start. The Next.js docs for output: "standalone" explicitly state that a minimal server.js is output and can be used instead of next start, and they also provide the command node.next/standalone/server.js. [1] Next.js also has an explicit warning that “next start” does not work with “output: standalone” and instructs you to use “node.next/standalone/server.js” instead. [2] So: - Works: node.next/standalone/server.js [1] - Not recommended / not supported for standalone output: next start [2][1]
Citations:
- 1: https://nextjs.org/docs/pages/api-reference/config/next-config-js/output
- 2: Warning needs clarification: `⚠ "next start" does not work with "output: standalone" ...` vercel/next.js#68281
Fix npm run start for Next.js output: 'standalone'
src/platform/next.config.mjs uses output: 'standalone', but src/platform/package.json sets "start": "next start". With standalone output, Next.js doesn’t support next start; it must be started with node .next/standalone/server.js. This mismatch also conflicts with src/platform/README.md “Production Mode” instructions.
Current
"start": "next start",Update the start script to node .next/standalone/server.js (and keep the Dockerfile as-is, since it already runs node server.js from .next/standalone).
🤖 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/package.json` at line 8, The package.json "start" script is
wrong for Next.js when next.config.mjs uses output: 'standalone'; update the
start script in src/platform/package.json so it runs the standalone server (node
.next/standalone/server.js) instead of "next start" to match next.config.mjs and
the README.md "Production Mode" instructions and keep compatibility with the
Dockerfile that runs server.js from .next/standalone.
Contributor
Author
There was a problem hiding this comment.
Still valid according to the nextjs docs on this no change needed here.
Contributor
There was a problem hiding this comment.
@OchiengPaul442, fair point — and worth noting that the production Dockerfile already handles this correctly by running node server.js directly from .next/standalone, so the package.json start script won't be invoked in containerized deployments anyway. If next start works for your local dev/testing workflow per the docs you're referencing, that's a reasonable call. Acknowledged.
🐇 ╮(╯_╰)╭
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/platform/src/modules/analytics/utils/index.ts (1)
245-255: 💤 Low valueField priority order differs from shared
getSiteDisplayName.This helper uses
namebeforelocation_name, whereas the sharedgetSiteDisplayNameutility (insiteUtils.ts) prioritizeslocation_namebeforename. If this is intentional forsiteDetailsfrom recent readings, consider adding a brief comment explaining the rationale. If not, aligning with the shared utility would ensure consistent display names across the codebase.🤖 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/utils/index.ts` around lines 245 - 255, The getRecentReadingSiteName function currently chooses siteDetails.name before siteDetails.location_name which conflicts with the shared getSiteDisplayName ordering; either reorder the priority in getRecentReadingSiteName to match getSiteDisplayName (use location_name before name) or, if the different order is intentional for RecentReading data, add a concise comment above getRecentReadingSiteName explaining why its priority differs and referencing getSiteDisplayName for clarity.
🤖 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/shared/providers/UserDataFetcher.tsx`:
- Around line 171-177: The LoadingOverlay in UserDataFetcher.tsx currently uses
delayMs={0} causing immediate full-screen flicker; change it to a short debounce
(e.g., 150–300ms) or replace with a subtler inline spinner to avoid white flash
by the LoadingOverlay component, and confirm activeGroupMissingFromFreshGroups
logic still resolves when setGroups in userSlice.ts updates activeGroup (no
change needed there) so the overlay won’t deadlock—update the LoadingOverlay
usage (symbol: LoadingOverlay in UserDataFetcher.tsx) to use a non-zero delay or
subtler UI.
---
Nitpick comments:
In `@src/platform/src/modules/analytics/utils/index.ts`:
- Around line 245-255: The getRecentReadingSiteName function currently chooses
siteDetails.name before siteDetails.location_name which conflicts with the
shared getSiteDisplayName ordering; either reorder the priority in
getRecentReadingSiteName to match getSiteDisplayName (use location_name before
name) or, if the different order is intentional for RecentReading data, add a
concise comment above getRecentReadingSiteName explaining why its priority
differs and referencing getSiteDisplayName for clarity.
🪄 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: 5d34e7d3-a367-41f2-97e9-cbb4291d6d91
📒 Files selected for processing (10)
src/platform/src/modules/analytics/components/AnalyticsDashboard.tsxsrc/platform/src/modules/analytics/hooks/index.tssrc/platform/src/modules/analytics/utils/index.tssrc/platform/src/shared/components/charts/components/charts/DynamicChart.tsxsrc/platform/src/shared/components/header/index.tsxsrc/platform/src/shared/components/ui/bottom-navigation.tsxsrc/platform/src/shared/layouts/MainLayout.tsxsrc/platform/src/shared/providers/UserDataFetcher.tsxsrc/platform/src/shared/services/analyticsService.tssrc/platform/src/shared/services/apiClient.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/platform/src/shared/services/analyticsService.ts
Comment on lines
+171
to
+177
| if ( | ||
| hasStalePersistedUser || | ||
| (status === 'authenticated' && !!userId && isLoading && !data) || | ||
| activeGroupMissingFromFreshGroups | ||
| ) { | ||
| return <LoadingOverlay delayMs={0} />; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's external logic that handles stale/invalid activeGroup clearing
rg -n "activeGroup" --type=ts -g '!*.test.*' -g '!*.spec.*' -A3 -B3 | head -100Repository: airqo-platform/AirQo-frontend
Length of output: 7212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect UserDataFetcher gating logic + surrounding logout logic
sed -n '130,220p' src/platform/src/shared/providers/UserDataFetcher.tsx | cat -n
# 2) Find where activeGroupMissingFromFreshGroups is defined/computed
rg -n "activeGroupMissingFromFreshGroups" src/platform/src/shared/providers/UserDataFetcher.tsx -n
rg -n "activeGroupMissingFromFreshGroups" src/platform/src -g'!*.test.*' -g'!*.spec.*'
# 3) Inspect userSlice reducers for any "sync activeGroup with groups" behavior
sed -n '140,320p' src/vertex/core/redux/slices/userSlice.ts | cat -n
# 4) Search for any action/reducer that clears activeGroup when group list changes
rg -n "setActiveGroup|userGroups|activeGroup" src/vertex/core/redux/slices/userSlice.ts -nRepository: airqo-platform/AirQo-frontend
Length of output: 11454
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the full block where activeGroupMissingFromFreshGroups is computed and used (plus nearby effects)
sed -n '1,220p' src/platform/src/shared/providers/UserDataFetcher.tsx | cat -n | sed -n '1,220p'
# Focused searches inside this file for any mismatch-handling
rg -n "activeGroupMissingFromFreshGroups" src/platform/src/shared/providers/UserDataFetcher.tsx
rg -n "setActiveGroup\\(|activeGroup" src/platform/src/shared/providers/UserDataFetcher.tsx
rg -n "\\blogout\\(" src/platform/src/shared/providers/UserDataFetcher.tsx
# Confirm whether logout reducer clears activeGroup in userSlice.ts
rg -n "logout\\(state" -n src/vertex/core/redux/slices/userSlice.ts
sed -n '1,140p' src/vertex/core/redux/slices/userSlice.ts | cat -nRepository: airqo-platform/AirQo-frontend
Length of output: 12804
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate selectActiveGroup implementation
rg -n "selectActiveGroup" src/platform/src/shared/store -g'*.ts*'
# 2) Inspect shared userSlice reducers for setGroups/logout affecting activeGroup
rg -n "setGroups\\b|logout\\b|activeGroup" src/platform/src/shared/store/userSlice.ts -n
# 3) Print relevant sections from shared userSlice (setGroups + logout + any activeGroup sync)
sed -n '1,260p' src/platform/src/shared/store/userSlice.ts | cat -nRepository: airqo-platform/AirQo-frontend
Length of output: 4795
Reduce LoadingOverlay flicker; mismatch won’t deadlock
delayMs={0}makes the overlay appear immediately; adjust to a small delay (e.g., 150–300ms) or use a subtler loading UI to avoid full-white flicker.- The “infinite loading” scenario is unlikely: when fresh groups arrive,
setGroupsinsrc/platform/src/shared/store/userSlice.tspreservesactiveGrouponly if it exists in the new list; otherwise it switchesactiveGroupto the AIRQO group (or first group) or sets it tonull, which clearsactiveGroupMissingFromFreshGroupson the next render.
🤖 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/providers/UserDataFetcher.tsx` around lines 171 -
177, The LoadingOverlay in UserDataFetcher.tsx currently uses delayMs={0}
causing immediate full-screen flicker; change it to a short debounce (e.g.,
150–300ms) or replace with a subtler inline spinner to avoid white flash by the
LoadingOverlay component, and confirm activeGroupMissingFromFreshGroups logic
still resolves when setGroups in userSlice.ts updates activeGroup (no change
needed there) so the overlay won’t deadlock—update the LoadingOverlay usage
(symbol: LoadingOverlay in UserDataFetcher.tsx) to use a non-zero delay or
subtler UI.
… handling and improve user preferences integration
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/platform/src/modules/airqo-map/components/sidebar/LocationsList.tsx (2)
246-257: 💤 Low valueThe
shouldFetchSitesconditional is dense; consider extracting for clarity.This boolean expression chains multiple conditions with different semantic meanings (searching, country filtering, preference resolution). It works correctly, but readability would benefit from either inline comments or extracting named sub-conditions.
That said, the logic is sound and tested via the consuming code paths. Optional refinement if you revisit this area.
🤖 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/airqo-map/components/sidebar/LocationsList.tsx` around lines 246 - 257, The complex boolean assigned to shouldFetchSites is hard to read; refactor by extracting meaningful named sub-conditions (e.g., isFilteringByCountry, needsFavoriteResolution, hasNoFavorites, isActiveSearch) and then compose shouldFetchSites from those names so the logic is self-documenting; update the call-site to use the new shouldFetchSites and keep the same behavior for useSitesByCountry (referencing shouldFetchSites, selectedCountry, isSearching, shouldResolveInitialFavorites, isWaitingForFavoriteDecision, hasPreferenceContext, favoriteLocations.length).
80-90: 💤 Low valueNull-island filtering treats (0, 0) as invalid coordinates.
The function returns
nullwhen both latitude and longitude are exactly 0. While (0, 0) is a valid geographic coordinate (Gulf of Guinea), it's an extremely unlikely location for AirQo monitoring sites. This appears to be intentional null-island/default-value filtering.If this is indeed the intent, a brief comment would clarify for future readers:
// Reject (0, 0) as a likely default/unset value rather than a real location if (latitude === 0 && longitude === 0) { return null; }No action required if the behavior is intentional—just a clarity suggestion.
🤖 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/airqo-map/components/sidebar/LocationsList.tsx` around lines 80 - 90, The getUsableCoordinates function currently treats latitude===0 && longitude===0 as invalid and returns null; if this is intentional as a "null‑island" / default-unset filter, add a brief clarifying comment directly above that conditional in getUsableCoordinates explaining "Reject (0,0) as a likely default/unset value rather than a real location (null‑island)". Leave the logic unchanged but document the intent so future readers understand why (0,0) is excluded.src/platform/src/modules/airqo-map/hooks/useMapReadings.ts (1)
48-55: ⚡ Quick winAuto-retry policy conflicts with coding guidelines.
The
shouldAutoRetryMapReadingsfunction triggers retries for both abort-like errors and 5xx responses. The coding guidelines explicitly state to "Avoid retrying 5xx, ERR_NETWORK, or aborted requests to prevent retry storms."While the implementation sensibly limits retries to one per cohort key (via
autoRetriedKeyRef), this still creates an implicit retry for classes of errors the guidelines recommend against. Consider either:
- Removing the auto-retry for these error classes entirely, or
- Documenting why a single bounded retry is acceptable here (e.g., transient network hiccups on initial load)
If option 2, a brief comment explaining the deviation would help future maintainers.
🤖 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/airqo-map/hooks/useMapReadings.ts` around lines 48 - 55, The auto-retry logic in shouldAutoRetryMapReadings currently retries for abort-like errors and 5xx status codes which conflicts with the coding guideline to avoid retrying aborted, network, or 5xx responses; update the shouldAutoRetryMapReadings function so it does not trigger retries for these classes of errors (e.g., always return false for isAbortLikeError(error) and for status codes >=500 && <600), or if you intentionally permit a single bounded retry, add a concise comment above shouldAutoRetryMapReadings explaining the exception and why one retry is acceptable (mentioning the cohort-key single-retry bound via autoRetriedKeyRef and the transient-initial-load rationale) so future maintainers understand the deviation.
🤖 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/airqo-map/hooks/useMapReadings.ts`:
- Around line 140-145: The return currently exposes error?.message which can
surface abort/cancellation messages to users; update useMapReadings so the
returned error is null for cancellation errors by filtering them before exposing
to UI—add a small predicate (e.g., isCancellationError) that checks common
signals (Axios cancel/ isCancel, error.name === 'CanceledError' or 'AbortError',
or error.message === 'canceled') and use it when constructing the returned error
field (keep readings, isLoading, and refetch logic unchanged).
---
Nitpick comments:
In `@src/platform/src/modules/airqo-map/components/sidebar/LocationsList.tsx`:
- Around line 246-257: The complex boolean assigned to shouldFetchSites is hard
to read; refactor by extracting meaningful named sub-conditions (e.g.,
isFilteringByCountry, needsFavoriteResolution, hasNoFavorites, isActiveSearch)
and then compose shouldFetchSites from those names so the logic is
self-documenting; update the call-site to use the new shouldFetchSites and keep
the same behavior for useSitesByCountry (referencing shouldFetchSites,
selectedCountry, isSearching, shouldResolveInitialFavorites,
isWaitingForFavoriteDecision, hasPreferenceContext, favoriteLocations.length).
- Around line 80-90: The getUsableCoordinates function currently treats
latitude===0 && longitude===0 as invalid and returns null; if this is
intentional as a "null‑island" / default-unset filter, add a brief clarifying
comment directly above that conditional in getUsableCoordinates explaining
"Reject (0,0) as a likely default/unset value rather than a real location
(null‑island)". Leave the logic unchanged but document the intent so future
readers understand why (0,0) is excluded.
In `@src/platform/src/modules/airqo-map/hooks/useMapReadings.ts`:
- Around line 48-55: The auto-retry logic in shouldAutoRetryMapReadings
currently retries for abort-like errors and 5xx status codes which conflicts
with the coding guideline to avoid retrying aborted, network, or 5xx responses;
update the shouldAutoRetryMapReadings function so it does not trigger retries
for these classes of errors (e.g., always return false for
isAbortLikeError(error) and for status codes >=500 && <600), or if you
intentionally permit a single bounded retry, add a concise comment above
shouldAutoRetryMapReadings explaining the exception and why one retry is
acceptable (mentioning the cohort-key single-retry bound via autoRetriedKeyRef
and the transient-initial-load rationale) so future maintainers understand the
deviation.
🪄 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: 652559ca-2e3d-4a80-af66-a249d0469369
📒 Files selected for processing (3)
src/platform/src/modules/airqo-map/MapPage.tsxsrc/platform/src/modules/airqo-map/components/sidebar/LocationsList.tsxsrc/platform/src/modules/airqo-map/hooks/useMapReadings.ts
Comment on lines
140
to
145
| return { | ||
| readings, | ||
| isLoading, | ||
| isLoading: isLoading || (readings.length === 0 && isFetching), | ||
| error: error?.message ?? null, | ||
| refetch, | ||
| }; |
Contributor
There was a problem hiding this comment.
Abort/cancellation errors may still surface to users.
Line 143 returns error?.message ?? null, which means if a request is cancelled (e.g., due to component unmount or cohort change), the abort error message ("canceled") could be displayed to users. The coding guidelines specify: "Ensure cancellation errors are not surfaced to users as real failures."
Consider filtering abort-like errors before exposing them:
Proposed fix
return {
readings,
isLoading: isLoading || (readings.length === 0 && isFetching),
- error: error?.message ?? null,
+ error: error && !isAbortLikeError(error) ? error.message : null,
refetch,
};🤖 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/airqo-map/hooks/useMapReadings.ts` around lines 140
- 145, The return currently exposes error?.message which can surface
abort/cancellation messages to users; update useMapReadings so the returned
error is null for cancellation errors by filtering them before exposing to
UI—add a small predicate (e.g., isCancellationError) that checks common signals
(Axios cancel/ isCancel, error.name === 'CanceledError' or 'AbortError', or
error.message === 'canceled') and use it when constructing the returned error
field (keep readings, isLoading, and refetch logic unchanged).
Contributor
|
New azure analytics_platform changes available for preview here |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the analytics/map “recent readings” and favorites flows, improves token refresh behavior, and adjusts layout/navigation behavior to better support PR preview deployments.
Changes:
- Improve analytics site cards by fetching recent readings (with batching + fallback to chart-data) and enriching site-card location metadata.
- Harden auth/session handling: proactive JWT refresh in the API client and safer callback URL normalization after login.
- PR preview/deployment updates for analytics platform (dynamic
NEXTAUTH_URLinjection + health check), plus map and layout refinements (favorites-aware locations list, map readings retry).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platform/src/shared/types/api.ts | Removes user_id from recent readings request type. |
| src/platform/src/shared/services/deviceService.ts | Adds cohort-details fallback behavior; removes user_id from map readings token call. |
| src/platform/src/shared/services/apiClient.ts | Adds JWT expiry detection + proactive refresh and consolidates refresh logic. |
| src/platform/src/shared/services/analyticsService.ts | Batches recent-readings requests and normalizes varied payload shapes. |
| src/platform/src/shared/providers/UserDataFetcher.tsx | Adds guarded loading overlay to prevent stale user/group state flashes. |
| src/platform/src/shared/layouts/MainLayout.tsx | Adjusts main scroll/padding to work with non-fixed bottom nav. |
| src/platform/src/shared/components/ui/bottom-navigation.tsx | Simplifies bottom nav (no scroll-hide), changes positioning. |
| src/platform/src/shared/components/header/index.tsx | Updates header positioning to match new layout scroll model. |
| src/platform/src/shared/components/charts/components/charts/DynamicChart.tsx | Tweaks axis tick interval behavior for readability. |
| src/platform/src/modules/analytics/utils/index.ts | Enriches recent-reading normalization (names, location fields, no-value handling). |
| src/platform/src/modules/analytics/hooks/index.ts | Uses recent readings for site cards with compatibility fallback; normalizes preference sites. |
| src/platform/src/modules/analytics/components/AnalyticsDashboard.tsx | Uses explicit country when present for “More insights” selection. |
| src/platform/src/modules/airqo-map/MapPage.tsx | Removes country hook linkage; keeps map filtering driven by state. |
| src/platform/src/modules/airqo-map/hooks/useMapReadings.ts | Adds one-shot auto-retry for transient failures + improved loading signaling. |
| src/platform/src/modules/airqo-map/components/sidebar/LocationsList.tsx | Adds favorites-aware list + improved skeleton/loading logic and coordinate handling. |
| src/platform/src/app/layout.tsx | Removes next/font Inter usage; uses font-sans. |
| src/platform/src/app/(dashboard)/(organization)/org/[org_slug]/(auth)/login/page.tsx | Normalizes callback URL before redirect. |
| src/platform/src/app/(dashboard)/(individual)/user/(auth)/login/page.tsx | Normalizes callback URL before redirect. |
| src/platform/README.md | Adds production build/start instructions. |
| src/platform/package.json | Simplifies start script to next start. |
| src/platform/Dockerfile | Removes explicit PORT/HOSTNAME env lines (keeps standalone server start). |
| .github/workflows/deploy-frontend-azurepreview.yml | Injects PR-preview auth env vars, validates secrets, adds health check, and refines env-var handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+73
to
+82
| normalizedSitesById.set(siteId, { | ||
| search_name: | ||
| selectedSite.search_name || | ||
| selectedSite.formatted_name || | ||
| selectedSite.generated_name || | ||
| selectedSite.name || | ||
| siteId, | ||
| ...selectedSite, | ||
| _id: siteId, | ||
| }); |
Comment on lines
328
to
335
| @@ -109,14 +333,17 @@ export const LocationsList: React.FC<LocationsListProps> = ({ | |||
| name: location.title, | |||
| }); | |||
| } else { | |||
Comment on lines
+375
to
+377
| BRANCH="${{ needs.branch-name.outputs.lowercase }}" | ||
| APP="${BRANCH:0:12}-analytics-p-preview" | ||
|
|
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
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
Chores
Documentation