feat: weather display at user location#149
Merged
Merged
Conversation
Show current in-game Pokemon GO weather on the dashboard with boosted type chips, severity warnings, and relative update timestamps. Reads weather data from the scanner DB weather table via S2 level-10 cell lookup. Gracefully hidden when scanner DB is not configured.
Contributor
Author
Code Review — PR !149: feat: weather display at user locationOverall Assessment
Requirements Traceability (Issue 128)
Code Review FindingsCritical IssuesNone. Major Issues (Should Fix)
Minor Issues (Consider)
Architecture & Design ReviewPattern Compliance: Excellent
S2CellHelper Design:
Frontend Integration:
Test Coverage
Missing test coverage (non-blocking):
Risk Assessment
Final Verdict: APPROVEDClean, well-structured feature that follows all existing patterns. The S2 cell computation is the most complex piece but is well-tested and follows the reference Google implementation. The optional scanner DB dependency means zero impact on deployments without a scanner database. Pre-merge: No blockers.
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| lon = human.Longitude; | ||
| } | ||
|
|
||
| if (lat == 0 && lon == 0) |
| lon = human.Longitude; | ||
| } | ||
|
|
||
| if (lat == 0 && lon == 0) |
- Fixed S2CellHelper lookup table initialization: origOrientation must match orientation in init calls (ported from Go S2 cellid.go) - Fixed kPosToIJ Hilbert curve table to use correct inverse mapping - Rewrote FaceIjToCell using Go S2 approach (single 64-bit accumulator with face at bit 60) instead of C++ n[0]/n[1] halving - Changed weather endpoint from 404 to 204 No Content for missing data to avoid triggering the global error interceptor toast
Each area chip in the "Tracking areas" section now displays a weather icon with a tooltip showing the condition and boosted types. Uses the area's geofence polygon centroid to resolve the S2 weather cell. Backend: POST /api/location/weather/areas batch endpoint accepts area centroids, deduplicates S2 cells, returns weather per area. Frontend: forkJoin areas + geofence polygons, compute centroids via geo.utils, batch-request weather, display on area chips.
|
|
||
| // Compute S2 cell IDs for each location, deduplicating cells | ||
| var locationCells = request.Locations | ||
| .Where(l => l.Lat != 0 || l.Lon != 0) |
|
|
||
| // Compute S2 cell IDs for each location, deduplicating cells | ||
| var locationCells = request.Locations | ||
| .Where(l => l.Lat != 0 || l.Lon != 0) |
Contributor
Author
Code Review — PR !149 (Updated): Weather Display at User LocationReviewing 3 commits, 14 files, +1,024 / -10 lines Overall Assessment
Commits
Requirements Traceability (Issue 128)
Code Review FindingsCritical IssuesNone. Major IssuesNone. The S2 cell computation bug (wrong Hilbert curve tables, wrong init params) was caught and fixed in commit `d60115e`. The 404→204 fix for the error interceptor toast was also addressed. Minor Issues (Consider)
Architecture & DesignS2CellHelper — Clean port from Go S2 library (`golang/geo`). Uses single 64-bit accumulator approach (simpler than C++ n[0]/n[1] halving). Lookup tables derived from reference `kIJtoPos` inverse with `kPosToOrientation` XOR deltas. Verified against Golbat's scanner DB cell IDs. Batch weather endpoint — Smart design: frontend computes centroids from cached geofence polygons, sends batch to single `POST` endpoint. Backend deduplicates S2 cells before DB query. Nearby areas sharing a weather cell = single row fetch. 204 No Content — Good choice over 404 for "no data" cases. Avoids triggering the global error interceptor toast while still allowing the frontend to handle gracefully (Angular HttpClient returns null body for 204). Frontend signals — `weather`, `weatherLoading`, `weatherUpdatedAgo`, `areaWeather` all follow existing dashboard signal patterns. `forkJoin` for areas + geofences is clean. Test Coverage
Not covered (non-blocking): controller endpoint tests, batch weather endpoint tests, frontend Jest tests. Risk Assessment
Verdict: APPROVEDWell-executed feature with clean architecture. The S2 cell bug was caught during live testing and properly fixed. Per-area weather is a nice bonus that reuses existing geofence polygon infrastructure. All CI checks pass. |
hokiepokedad2
added a commit
that referenced
this pull request
Apr 7, 2026
5 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
1 participant
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.
Summary
weathertable via S2 level-10 cell ID lookup (primary key, single row fetch)Changes
Backend (5 new files, 4 modified)
weathertableGetWeatherAtLocationAsync()querying weather by computed cell IDGET /api/location/weatherendpoint; scanner service is optional (null when not configured)Frontend (3 modified, 1 new model)
LocationService.getWeather()methodTest Plan
Closes #128