Address code review findings for weather coordinates feature
- CRIT-1: Add lat/lon validation ([-90,90] and [-180,180]) in Pydantic schema - WARN-1: Replace deprecated get_event_loop() with get_running_loop() - SUG-1: Add GeoSearchResult response model to /search endpoint - SUG-2: Dashboard weather query enables on coordinates too, not just city - SUG-3: Clean up debounce timer on component unmount - SUG-4: Fix geocoding URL from HTTP to HTTPS - SUG-5: Add fallback display when weather_city is null but coords exist Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
1545da48e5
commit
4b5649758a
281
.claude/QA/weather_coordinates_review.md
Normal file
281
.claude/QA/weather_coordinates_review.md
Normal file
@ -0,0 +1,281 @@
|
||||
# Weather Coordinate-Based Lookup - Code Review
|
||||
|
||||
**Feature:** Replace plain-text weather city input with geocoding search + coordinate-based weather fetch
|
||||
**Reviewer:** Senior Code Reviewer
|
||||
**Date:** 2026-02-21
|
||||
**Files reviewed:** 6
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
This feature replaces the free-text weather city input with a search-and-select location picker backed by OpenWeatherMap's geocoding API. Settings now store `weather_lat` and `weather_lon` alongside `weather_city`, and the weather endpoint prefers coordinates over city name for more accurate results. The implementation is solid overall -- the backend correctly uses `run_in_executor` for blocking I/O (addressing CRIT-2 from the Phase 2 review), the cache now keys on coordinates, and the frontend location picker is well-crafted with debounced search, click-outside dismissal, and proper cache invalidation. However, there are a few issues around input validation, deprecated API usage, and a previous review finding (WARN-1) that was only partially addressed.
|
||||
|
||||
---
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### [CRIT-1] No validation on latitude/longitude values in schema
|
||||
|
||||
**File:** `backend/app/schemas/settings.py` (lines 30-31)
|
||||
|
||||
```python
|
||||
weather_lat: float | None = None
|
||||
weather_lon: float | None = None
|
||||
```
|
||||
|
||||
Latitude must be in [-90, 90] and longitude in [-180, 180]. The schema accepts any float, meaning a malformed or malicious request could send `weather_lat: 99999` which would be persisted to the database and forwarded to the OpenWeatherMap API. While the API would reject it, the invalid data would be stored permanently.
|
||||
|
||||
**Fix:** Add field validators:
|
||||
```python
|
||||
from pydantic import field_validator
|
||||
|
||||
class SettingsUpdate(BaseModel):
|
||||
# ... existing fields ...
|
||||
weather_lat: float | None = None
|
||||
weather_lon: float | None = None
|
||||
|
||||
@field_validator('weather_lat')
|
||||
@classmethod
|
||||
def validate_lat(cls, v: float | None) -> float | None:
|
||||
if v is not None and (v < -90 or v > 90):
|
||||
raise ValueError('Latitude must be between -90 and 90')
|
||||
return v
|
||||
|
||||
@field_validator('weather_lon')
|
||||
@classmethod
|
||||
def validate_lon(cls, v: float | None) -> float | None:
|
||||
if v is not None and (v < -180 or v > 180):
|
||||
raise ValueError('Longitude must be between -180 and 180')
|
||||
return v
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Warnings
|
||||
|
||||
### [WARN-1] `asyncio.get_event_loop()` is deprecated in Python 3.12
|
||||
|
||||
**File:** `backend/app/routers/weather.py` (lines 36, 85)
|
||||
|
||||
```python
|
||||
loop = asyncio.get_event_loop()
|
||||
results = await loop.run_in_executor(None, _fetch_json, url)
|
||||
```
|
||||
|
||||
`asyncio.get_event_loop()` emits a deprecation warning in Python 3.10+ when called outside of an async context, and its behavior is unreliable. Inside an async function the running loop is available, but the idiomatic replacement is `asyncio.get_running_loop()`:
|
||||
|
||||
```python
|
||||
loop = asyncio.get_running_loop()
|
||||
```
|
||||
|
||||
This is a simple 1-word change on two lines and future-proofs the code.
|
||||
|
||||
### [WARN-2] Cache invalidation on location change is implicit, not explicit
|
||||
|
||||
**File:** `backend/app/routers/weather.py` (lines 72-78)
|
||||
|
||||
```python
|
||||
cache_key = f"{lat},{lon}" if use_coords else city
|
||||
if _cache.get("expires_at") and now < _cache["expires_at"] and _cache.get("cache_key") == cache_key:
|
||||
return _cache["data"]
|
||||
```
|
||||
|
||||
The Phase 2 review flagged WARN-1 (cache not invalidated on city change). This implementation improves it by keying the cache on coordinates/city, so a location change will naturally miss the cache. This is good. However, the old cached data still occupies memory until the next fetch overwrites it -- a minor concern for a single-user app but worth documenting.
|
||||
|
||||
Additionally, using a plain `dict` with string keys is fragile. If `lat` or `lon` is `None` due to a race or unexpected state, the cache key becomes `"None,None"` which could match incorrectly.
|
||||
|
||||
**Recommendation:** Add an explicit check:
|
||||
```python
|
||||
if lat is None or lon is None:
|
||||
cache_key = city
|
||||
else:
|
||||
cache_key = f"{lat},{lon}"
|
||||
```
|
||||
|
||||
Wait -- this is already handled by the `use_coords` boolean. The current logic is correct. Disregard the race concern; the ternary is safe.
|
||||
|
||||
### [WARN-3] Search endpoint lacks rate limiting
|
||||
|
||||
**File:** `backend/app/routers/weather.py` (lines 26-53)
|
||||
|
||||
The `/search` endpoint calls OpenWeatherMap's geocoding API on every request. The frontend debounces at 300ms, but there's no backend throttle. A user rapidly typing triggers multiple API calls. For a single-user app this is unlikely to hit OWM's rate limits, but the 300ms debounce on the frontend is the only protection.
|
||||
|
||||
**Recommendation:** This is acceptable for a single-user app. No action needed unless OWM rate limit errors appear in production.
|
||||
|
||||
### [WARN-4] Clearing location sends `null` but `SettingsUpdate` uses `Optional` with `exclude_unset`
|
||||
|
||||
**File:** `frontend/src/components/settings/SettingsPage.tsx` (lines 86-98)
|
||||
**File:** `backend/app/routers/settings.py` (line 29)
|
||||
|
||||
```typescript
|
||||
await updateSettings({
|
||||
weather_city: null,
|
||||
weather_lat: null,
|
||||
weather_lon: null,
|
||||
});
|
||||
```
|
||||
|
||||
The backend uses `model_dump(exclude_unset=True)`, so explicitly sending `null` values will include them in the update dict -- which correctly sets the DB columns to `NULL`. This works as intended. However, this relies on the frontend correctly sending `null` rather than omitting the keys. If `updateSettings` ever strips `null` values before sending, the clear operation would silently fail.
|
||||
|
||||
**Recommendation:** This works correctly today. Add a comment in `handleLocationClear` noting that explicit `null` is required for the backend to clear the fields.
|
||||
|
||||
### [WARN-5] `Float` column precision for coordinates
|
||||
|
||||
**File:** `backend/app/models/settings.py` (lines 16-17)
|
||||
|
||||
```python
|
||||
weather_lat: Mapped[float | None] = mapped_column(Float, nullable=True, default=None)
|
||||
weather_lon: Mapped[float | None] = mapped_column(Float, nullable=True, default=None)
|
||||
```
|
||||
|
||||
SQLAlchemy's `Float` maps to PostgreSQL `DOUBLE PRECISION` (8 bytes, ~15 digits). For geographic coordinates this is more than adequate (~1mm precision). No issue here -- just documenting that the type choice is appropriate.
|
||||
|
||||
---
|
||||
|
||||
## Suggestions
|
||||
|
||||
### [SUG-1] Weather search response should use a Pydantic model
|
||||
|
||||
**File:** `backend/app/routers/weather.py` (lines 26-49)
|
||||
|
||||
The `/search` endpoint returns a raw list of dicts. Adding a response model would document the API and catch shape issues:
|
||||
|
||||
```python
|
||||
from pydantic import BaseModel
|
||||
|
||||
class GeoSearchResult(BaseModel):
|
||||
name: str
|
||||
state: str
|
||||
country: str
|
||||
lat: float
|
||||
lon: float
|
||||
|
||||
@router.get("/search", response_model=list[GeoSearchResult])
|
||||
```
|
||||
|
||||
This mirrors the Phase 2 review's SUG-7 recommendation for the main weather endpoint.
|
||||
|
||||
### [SUG-2] Dashboard weather query should check coordinates, not just city
|
||||
|
||||
**File:** `frontend/src/components/dashboard/DashboardPage.tsx` (line 77)
|
||||
|
||||
```typescript
|
||||
enabled: !!settings?.weather_city,
|
||||
```
|
||||
|
||||
The backend now supports coordinate-based lookup without a city name (line 68-69 of weather.py checks both). The frontend enable condition should match:
|
||||
|
||||
```typescript
|
||||
enabled: !!(settings?.weather_city || (settings?.weather_lat != null && settings?.weather_lon != null)),
|
||||
```
|
||||
|
||||
In practice, the current `handleLocationSelect` always sets all three (`weather_city`, `weather_lat`, `weather_lon`) together, so this is unlikely to diverge. But for correctness, the enable condition should match what the backend accepts.
|
||||
|
||||
### [SUG-3] Debounce cleanup on unmount
|
||||
|
||||
**File:** `frontend/src/components/settings/SettingsPage.tsx` (lines 34, 64-65)
|
||||
|
||||
```typescript
|
||||
const debounceRef = useRef<ReturnType<typeof setTimeout>>();
|
||||
// ...
|
||||
if (debounceRef.current) clearTimeout(debounceRef.current);
|
||||
debounceRef.current = setTimeout(() => searchLocations(value), 300);
|
||||
```
|
||||
|
||||
If the component unmounts while a debounce timer is pending, the callback fires on an unmounted component, potentially causing a React state update warning. Add cleanup:
|
||||
|
||||
```typescript
|
||||
useEffect(() => {
|
||||
return () => {
|
||||
if (debounceRef.current) clearTimeout(debounceRef.current);
|
||||
};
|
||||
}, []);
|
||||
```
|
||||
|
||||
### [SUG-4] Search endpoint geocoding URL uses HTTP, not HTTPS
|
||||
|
||||
**File:** `backend/app/routers/weather.py` (line 37)
|
||||
|
||||
```python
|
||||
url = f"http://api.openweathermap.org/geo/1.0/direct?q={urllib.parse.quote(q)}&limit=5&appid={api_key}"
|
||||
```
|
||||
|
||||
The geocoding endpoint uses `http://` while the weather endpoints (lines 93-94) use `https://`. The API key is sent in the query string over an unencrypted connection. This means the API key could be intercepted in transit.
|
||||
|
||||
**Fix:** Change to `https://`:
|
||||
```python
|
||||
url = f"https://api.openweathermap.org/geo/1.0/direct?q={urllib.parse.quote(q)}&limit=5&appid={api_key}"
|
||||
```
|
||||
|
||||
### [SUG-5] `weather_city` display could be null when coordinates exist
|
||||
|
||||
**File:** `frontend/src/components/settings/SettingsPage.tsx` (line 275)
|
||||
|
||||
```tsx
|
||||
{settings?.weather_city}
|
||||
```
|
||||
|
||||
If somehow `weather_lat` and `weather_lon` are set but `weather_city` is null (e.g., direct API call), the location badge would display empty text. Add a fallback:
|
||||
|
||||
```tsx
|
||||
{settings?.weather_city || `${settings?.weather_lat}, ${settings?.weather_lon}`}
|
||||
```
|
||||
|
||||
### [SUG-6] Consider parallel fetch for weather current + forecast
|
||||
|
||||
**File:** `backend/app/routers/weather.py` (lines 96-99)
|
||||
|
||||
```python
|
||||
current_data, forecast_data = await asyncio.gather(
|
||||
loop.run_in_executor(None, _fetch_json, current_url),
|
||||
loop.run_in_executor(None, _fetch_json, forecast_url),
|
||||
)
|
||||
```
|
||||
|
||||
This is already using `asyncio.gather` for parallel execution -- great improvement from the Phase 2 sequential pattern. No action needed.
|
||||
|
||||
---
|
||||
|
||||
## Positive Observations
|
||||
|
||||
1. **CRIT-2 from Phase 2 is fully resolved.** Both the `/search` and `/` endpoints now use `run_in_executor` for blocking `urllib` calls, and the main weather endpoint uses `asyncio.gather` to fetch current + forecast in parallel. This is a significant improvement.
|
||||
|
||||
2. **Cache keying is well-designed.** The `cache_key` switches between `"{lat},{lon}"` and `city` based on what's available, correctly invalidating when the user switches locations without needing an explicit cache clear.
|
||||
|
||||
3. **Frontend UX is polished.** The search-and-select pattern with debounced input, loading spinner, click-outside dismissal, and clear button is a substantial UX improvement over a plain text input. The dropdown key uses `${loc.lat}-${loc.lon}-${i}` which handles duplicate city names correctly.
|
||||
|
||||
4. **Settings update is atomic.** `handleLocationSelect` sends `weather_city`, `weather_lat`, and `weather_lon` in a single `updateSettings` call, preventing partial state where coordinates exist without a city name.
|
||||
|
||||
5. **Migration is clean and reversible.** The Alembic migration correctly adds nullable float columns with a straightforward downgrade path.
|
||||
|
||||
6. **Type definitions stay in sync.** The `GeoLocation` interface and `Settings` type updates accurately mirror the backend schema changes.
|
||||
|
||||
7. **Error handling in the frontend is user-friendly.** Both `handleLocationSelect` and `handleLocationClear` catch errors and show toast notifications, and the search function silently clears results on failure rather than showing error UI.
|
||||
|
||||
---
|
||||
|
||||
## Summary of Action Items
|
||||
|
||||
| Priority | ID | Issue | Effort |
|
||||
|----------|------|-------|--------|
|
||||
| CRITICAL | CRIT-1 | No lat/lon validation in Pydantic schema | 15 lines |
|
||||
| WARNING | WARN-1 | Deprecated `get_event_loop()` - use `get_running_loop()` | 2 lines |
|
||||
| SUGGESTION | SUG-4 | Geocoding URL uses HTTP, leaks API key in transit | 1 line |
|
||||
| SUGGESTION | SUG-1 | No response_model on search endpoint | 10 lines |
|
||||
| SUGGESTION | SUG-2 | Dashboard enables weather query on city only, not coords | 1 line |
|
||||
| SUGGESTION | SUG-3 | Debounce timer not cleaned up on unmount | 4 lines |
|
||||
| SUGGESTION | SUG-5 | Location badge has no fallback for null city name | 1 line |
|
||||
|
||||
---
|
||||
|
||||
## Previous Review Items Status
|
||||
|
||||
| Phase 2 ID | Status | Notes |
|
||||
|------------|--------|-------|
|
||||
| CRIT-1 (double path prefix) | RESOLVED | Weather router now uses `@router.get("/")` |
|
||||
| CRIT-2 (blocking urllib) | RESOLVED | Uses `run_in_executor` + `asyncio.gather` |
|
||||
| WARN-1 (cache invalidation) | RESOLVED | Cache keys on coordinates/city |
|
||||
| WARN-2 (API key in errors) | RESOLVED | Error messages now use generic strings |
|
||||
| WARN-5 (stale settings state) | NOT ADDRESSED | Still present for other settings fields |
|
||||
| SUG-7 (no response_model) | NOT ADDRESSED | Still no Pydantic model for weather/search responses |
|
||||
@ -1,4 +1,5 @@
|
||||
from fastapi import APIRouter, Depends, HTTPException, Query
|
||||
from pydantic import BaseModel
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
from sqlalchemy import select
|
||||
from datetime import datetime, timedelta
|
||||
@ -18,12 +19,20 @@ router = APIRouter()
|
||||
_cache: dict = {}
|
||||
|
||||
|
||||
class GeoSearchResult(BaseModel):
|
||||
name: str
|
||||
state: str
|
||||
country: str
|
||||
lat: float
|
||||
lon: float
|
||||
|
||||
|
||||
def _fetch_json(url: str) -> dict:
|
||||
with urllib.request.urlopen(url, timeout=10) as resp:
|
||||
return json.loads(resp.read().decode())
|
||||
|
||||
|
||||
@router.get("/search")
|
||||
@router.get("/search", response_model=list[GeoSearchResult])
|
||||
async def search_locations(
|
||||
q: str = Query(..., min_length=1, max_length=100),
|
||||
current_user: Settings = Depends(get_current_session)
|
||||
@ -33,8 +42,8 @@ async def search_locations(
|
||||
raise HTTPException(status_code=500, detail="Weather API key not configured")
|
||||
|
||||
try:
|
||||
loop = asyncio.get_event_loop()
|
||||
url = f"http://api.openweathermap.org/geo/1.0/direct?q={urllib.parse.quote(q)}&limit=5&appid={api_key}"
|
||||
loop = asyncio.get_running_loop()
|
||||
url = f"https://api.openweathermap.org/geo/1.0/direct?q={urllib.parse.quote(q)}&limit=5&appid={api_key}"
|
||||
results = await loop.run_in_executor(None, _fetch_json, url)
|
||||
|
||||
return [
|
||||
@ -82,7 +91,7 @@ async def get_weather(
|
||||
raise HTTPException(status_code=500, detail="Weather API key not configured")
|
||||
|
||||
try:
|
||||
loop = asyncio.get_event_loop()
|
||||
loop = asyncio.get_running_loop()
|
||||
|
||||
# Build query params based on coordinates or city name
|
||||
if use_coords:
|
||||
|
||||
@ -30,6 +30,20 @@ class SettingsUpdate(BaseModel):
|
||||
weather_lat: float | None = None
|
||||
weather_lon: float | None = None
|
||||
|
||||
@field_validator('weather_lat')
|
||||
@classmethod
|
||||
def validate_lat(cls, v: float | None) -> float | None:
|
||||
if v is not None and (v < -90 or v > 90):
|
||||
raise ValueError('Latitude must be between -90 and 90')
|
||||
return v
|
||||
|
||||
@field_validator('weather_lon')
|
||||
@classmethod
|
||||
def validate_lon(cls, v: float | None) -> float | None:
|
||||
if v is not None and (v < -180 or v > 180):
|
||||
raise ValueError('Longitude must be between -180 and 180')
|
||||
return v
|
||||
|
||||
|
||||
class SettingsResponse(BaseModel):
|
||||
id: int
|
||||
|
||||
@ -74,7 +74,7 @@ export default function DashboardPage() {
|
||||
},
|
||||
staleTime: 30 * 60 * 1000,
|
||||
retry: false,
|
||||
enabled: !!settings?.weather_city,
|
||||
enabled: !!(settings?.weather_city || (settings?.weather_lat != null && settings?.weather_lon != null)),
|
||||
});
|
||||
|
||||
if (isLoading) {
|
||||
|
||||
@ -108,6 +108,13 @@ export default function SettingsPage() {
|
||||
return () => document.removeEventListener('mousedown', handler);
|
||||
}, []);
|
||||
|
||||
// Clean up debounce timer on unmount
|
||||
useEffect(() => {
|
||||
return () => {
|
||||
if (debounceRef.current) clearTimeout(debounceRef.current);
|
||||
};
|
||||
}, []);
|
||||
|
||||
const handleNameSave = async () => {
|
||||
const trimmed = preferredName.trim();
|
||||
if (trimmed === (settings?.preferred_name || '')) return;
|
||||
@ -272,7 +279,7 @@ export default function SettingsPage() {
|
||||
<div className="flex items-center gap-2">
|
||||
<span className="inline-flex items-center gap-2 rounded-md border border-accent/30 bg-accent/10 px-3 py-1.5 text-sm text-foreground">
|
||||
<MapPin className="h-3.5 w-3.5 text-accent" />
|
||||
{settings?.weather_city}
|
||||
{settings?.weather_city || `${settings?.weather_lat}, ${settings?.weather_lon}`}
|
||||
</span>
|
||||
<button
|
||||
type="button"
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user