From 374e07708f3ce087e387e97654d6783f4e7a1f49 Mon Sep 17 00:00:00 2001
From: Kyle Pope <kyle.a.pope@outlook.com>
Date: Fri, 20 Feb 2026 13:36:06 +0800
Subject: [PATCH] Fix QA review issues: route path, blocking I/O, API key leak,
 cache

- CRIT-1: Change weather route from /weather to / (was doubling prefix)
- CRIT-2: Use run_in_executor for urllib calls + parallel fetch
- WARN-1: Invalidate weather cache when city changes
- WARN-2: Sanitize error messages to prevent API key leakage
- SUG-2: Only enable weather query when city is configured
- SUG-4: Remove duplicate Bell import

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---
 backend/app/routers/weather.py                | 42 +++++++++++--------
 .../components/dashboard/DashboardPage.tsx    |  5 ++-
 2 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/backend/app/routers/weather.py b/backend/app/routers/weather.py
index 4321b69..45eabfc 100644
--- a/backend/app/routers/weather.py
+++ b/backend/app/routers/weather.py
@@ -2,6 +2,7 @@ from fastapi import APIRouter, Depends, HTTPException
 from sqlalchemy.ext.asyncio import AsyncSession
 from sqlalchemy import select
 from datetime import datetime, timedelta
+import asyncio
 import urllib.request
 import urllib.parse
 import urllib.error
@@ -17,16 +18,16 @@ router = APIRouter()
 _cache: dict = {}
 
 
-@router.get("/weather")
+def _fetch_json(url: str) -> dict:
+    with urllib.request.urlopen(url, timeout=10) as resp:
+        return json.loads(resp.read().decode())
+
+
+@router.get("/")
 async def get_weather(
     db: AsyncSession = Depends(get_db),
     current_user: Settings = Depends(get_current_session)
 ):
-    # Check cache
-    now = datetime.now()
-    if _cache.get("expires_at") and now < _cache["expires_at"]:
-        return _cache["data"]
-
     # Get city from settings
     result = await db.execute(select(Settings))
     settings_row = result.scalar_one_or_none()
@@ -34,20 +35,26 @@ async def get_weather(
     if not city:
         raise HTTPException(status_code=400, detail="No weather city configured")
 
+    # Check cache (also invalidate if city changed)
+    now = datetime.now()
+    if _cache.get("expires_at") and now < _cache["expires_at"] and _cache.get("city") == city:
+        return _cache["data"]
+
     api_key = app_settings.OPENWEATHERMAP_API_KEY
     if not api_key:
         raise HTTPException(status_code=500, detail="Weather API key not configured")
 
     try:
-        # Current weather
-        current_url = f"https://api.openweathermap.org/data/2.5/weather?q={urllib.parse.quote(city)}&units=metric&appid={api_key}"
-        with urllib.request.urlopen(current_url, timeout=10) as resp:
-            current_data = json.loads(resp.read().decode())
+        loop = asyncio.get_event_loop()
 
-        # Forecast for rain probability
+        # Current weather + forecast in parallel via thread pool
+        current_url = f"https://api.openweathermap.org/data/2.5/weather?q={urllib.parse.quote(city)}&units=metric&appid={api_key}"
         forecast_url = f"https://api.openweathermap.org/data/2.5/forecast?q={urllib.parse.quote(city)}&units=metric&cnt=8&appid={api_key}"
-        with urllib.request.urlopen(forecast_url, timeout=10) as resp:
-            forecast_data = json.loads(resp.read().decode())
+
+        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),
+        )
 
         rain_chance = 0
         for item in forecast_data.get("list", []):
@@ -69,10 +76,11 @@ async def get_weather(
         # Cache for 1 hour
         _cache["data"] = weather_result
         _cache["expires_at"] = now + timedelta(hours=1)
+        _cache["city"] = city
 
         return weather_result
 
-    except urllib.error.URLError as e:
-        raise HTTPException(status_code=502, detail=f"Weather service unavailable: {str(e)}")
-    except (KeyError, json.JSONDecodeError) as e:
-        raise HTTPException(status_code=502, detail=f"Invalid weather data: {str(e)}")
+    except urllib.error.URLError:
+        raise HTTPException(status_code=502, detail="Weather service unavailable")
+    except (KeyError, json.JSONDecodeError):
+        raise HTTPException(status_code=502, detail="Invalid weather data")
diff --git a/frontend/src/components/dashboard/DashboardPage.tsx b/frontend/src/components/dashboard/DashboardPage.tsx
index 272f8bf..34ff68c 100644
--- a/frontend/src/components/dashboard/DashboardPage.tsx
+++ b/frontend/src/components/dashboard/DashboardPage.tsx
@@ -1,7 +1,7 @@
 import { useState, useEffect, useRef } from 'react';
 import { useQuery } from '@tanstack/react-query';
 import { format } from 'date-fns';
-import { Bell, Plus, Calendar as CalIcon, ListTodo, Bell as BellIcon } from 'lucide-react';
+import { Bell, Plus, Calendar as CalIcon, ListTodo } from 'lucide-react';
 import api from '@/lib/api';
 import type { DashboardData, UpcomingResponse, WeatherData } from '@/types';
 import { useSettings } from '@/hooks/useSettings';
@@ -72,6 +72,7 @@ export default function DashboardPage() {
     },
     staleTime: 30 * 60 * 1000,
     retry: false,
+    enabled: !!settings?.weather_city,
   });
 
   if (isLoading) {
@@ -139,7 +140,7 @@ export default function DashboardPage() {
                 className="flex items-center gap-2.5 w-full px-3 py-2 text-sm hover:bg-card-elevated transition-colors"
                 onClick={() => { setQuickAddType('reminder'); setDropdownOpen(false); }}
               >
-                <BellIcon className="h-4 w-4 text-orange-400" />
+                <Bell className="h-4 w-4 text-orange-400" />
                 Reminder
               </button>
             </div>