chore: remove dead code and shrink over-engineered utilities (-390 lines) - #26
Conversation
…nes) - Delete ServiceRegistry (Dio clients built but never accessed; GetIt owns them) - Delete deprecation.py and its test (never wired to any prod route) - Shrink query_migration.py 159→18 lines (only check_deprecated_params was used) - Delete community_post_operations / community_comment_operations mixin stubs (pure pass-through wrappers; callers now import read/write classes directly) - Delete routes/posts.py aggregator; wire both post routers directly in main.py - Remove PaginatedResponse / DataResponse backward-compat aliases (no callers) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds weather snapshot schemas, Redis caching, backend API wiring, and frontend location-gated fallback handling with updated docs and config. It also splits community post routing, removes frontend ServiceRegistry setup, and trims shared legacy helpers. ChangesWeather snapshot feature
Community read/write split
Frontend service locator cleanup
Shared legacy helper cleanup
Sequence Diagram(s)sequenceDiagram
participant get_weather_snapshot
participant weather_service
participant RedisWeatherCache
participant OpenMeteoAPI
get_weather_snapshot->>weather_service: build_snapshot(request)
weather_service->>RedisWeatherCache: get(request)
RedisWeatherCache-->>weather_service: cached response or none
weather_service->>OpenMeteoAPI: GET /forecast
OpenMeteoAPI-->>weather_service: forecast payload
weather_service->>RedisWeatherCache: set(request, response)
weather_service-->>get_weather_snapshot: WeatherSnapshotResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)backend/main-backend/README.mdmarkdownlint-cli2 wrapper config was not available before execution 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 |
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@backend/main-backend/app/core/config.py`:
- Around line 53-60: The weather timeout and cache settings are not being
validated when the config model loads, so invalid values can slip through to
WeatherService and RedisWeatherCache.from_settings(). Add validation on the
settings class fields for open_meteo_timeout_seconds, weather_cache_ttl_seconds,
and weather_cache_distance_threshold to reject zero or negative values at load
time, using the existing config model identifiers in app/core/config.py.
In `@backend/main-backend/app/schemas/weather.py`:
- Around line 11-12: Add boundary validation to the Weather request schema so
invalid coordinates fail fast with a 422 instead of reaching the provider
lookup. Update the latitude and longitude fields in the Weather model to enforce
the valid ranges [-90, 90] and [-180, 180], using the existing schema definition
in weather.py so get_weather_snapshot() only receives validated coordinates.
In `@backend/main-backend/app/services/weather_cache.py`:
- Around line 114-120: The cache hit path in WeatherCache is reusing a full
WeatherSnapshotResponse, which can leak request-specific Strava temperature
values from a previous request. Update the WeatherCache response-building logic
to either cache only provider-derived weather data or, when returning a hit,
rebuild the snapshot with the current request’s strava_reported_celsius while
keeping the cached provider fields from cached.response.weather_snapshot. Use
the WeatherCache service flow and WeatherSnapshotResponse construction to locate
the fix.
In `@backend/main-backend/app/services/weather.py`:
- Around line 90-99: The weather provider response parsing in WeatherService
should treat malformed JSON as a provider-side error instead of letting it
escape as an unhandled exception. Update the fetch flow around the httpx.get,
response.raise_for_status, and response.json call in the weather service so that
ValueError/JSONDecodeError from response.json() is caught and converted into
WeatherServiceError, alongside the existing HTTPStatusError and HTTPError
handling, using the same WeatherServiceError path and logger.exception context
in the weather.py service.
- Around line 274-281: The _epoch_or_iso_to_utc helper in WeatherService
currently converts offset-less ISO timestamps via astimezone(UTC), which makes
naive datetimes follow the host timezone. Update the datetime parsing path in
_epoch_or_iso_to_utc so that when datetime.fromisoformat returns a value with
dt.tzinfo is None, it is treated as UTC by attaching UTC before conversion; keep
the existing handling for epoch strings and already-aware timestamps unchanged.
In `@backend/main-backend/README.md`:
- Around line 128-129: Add the missing WEATHER_CACHE_* environment variables to
the README settings list alongside the existing OPENMETEO_* entries, including
WEATHER_CACHE_ENABLED, WEATHER_CACHE_REDIS_URL, WEATHER_CACHE_NAMESPACE,
WEATHER_CACHE_TTL_SECONDS, and WEATHER_CACHE_DISTANCE_THRESHOLD, so the
configuration section in README reflects what the service actually reads.
In `@backend/main-backend/tests/test_weather.py`:
- Around line 102-121: The `test_weather_snapshot_integration_call_real_api`
test currently hits the live Open-Meteo service unconditionally, which makes CI
flaky. Update this test to either require an explicit integration marker or env
flag before calling `weather_service.build_snapshot`, or swap it to use a
stubbed/mocked provider response so it no longer depends on network or provider
uptime. Keep the change localized to the
`test_weather_snapshot_integration_call_real_api` flow and related weather test
setup.
In `@backend/shared/query_migration.py`:
- Line 7: The deprecated-name mapping in query_migration should not treat
activity_type as legacy because the /me route uses it as the active filter and
passes it through list_user_activities. Update the mapping in query_migration so
activity_type is removed from the deprecated-name set, keeping only the truly
old aliases like entity_type and kind, and make sure the route’s current filter
name remains unaffected.
In `@frontend/ios/Runner/Info.plist`:
- Around line 27-36: Remove the duplicated plist entries from Info.plist instead
of re-declaring them here. Keep the location permission keys only once, and
update the existing NSLocationAlwaysAndWhenInUseUsageDescription and
NSLocationWhenInUseUsageDescription values in the later section so the new
weather-related copy is the one iOS uses.
In `@frontend/lib/services/location_service.dart`:
- Around line 19-37: Update hasLocationAccess() in LocationService to match the
dual-source permission logic used by checkPermissions(), since it currently only
consults Geolocator and can disagree with Permission.location.status. Add the
same permission-source check before returning true so
ActivitiesContextRepository.getLocalWeather() only treats location as available
when both sources are aligned, and keep the existing service-enabled check after
that.
In `@packages/shared/openapi/openapi-main-weather.json`:
- Around line 4-14: Update the shared weather example to match the actual
WeatherService.build_snapshot contract. Replace the outdated OpenWeather/Strava
fields in the openapi example with the runtime shape returned by
WeatherService.build_snapshot, especially the source value (“Open-Meteo”) and
any provider-specific snapshot details such as condition code/icon URL. Keep the
example consistent with the actual response schema so consumers using this
mock/shared contract see the same payload that the service returns.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e47cbf2a-fa3c-4d5e-a922-fe2a66f83968
📒 Files selected for processing (32)
backend/README.mdbackend/community-backend/main.pybackend/community-backend/routes/posts.pybackend/community-backend/services/community_comment_operations.pybackend/community-backend/services/community_post_operations.pybackend/community-backend/services/supabase_client.pybackend/community-backend/tests/test_operations_units.pybackend/docker-compose.ymlbackend/main-backend/.env.examplebackend/main-backend/README.mdbackend/main-backend/app/api/v1/__init__.pybackend/main-backend/app/api/v1/weather.pybackend/main-backend/app/core/config.pybackend/main-backend/app/main.pybackend/main-backend/app/schemas/__init__.pybackend/main-backend/app/schemas/weather.pybackend/main-backend/app/services/__init__.pybackend/main-backend/app/services/weather.pybackend/main-backend/app/services/weather_cache.pybackend/main-backend/tests/test_weather.pybackend/shared/contracts.pybackend/shared/deprecation.pybackend/shared/query_migration.pybackend/tests/test_deprecation_middleware.pydocs/service-ownership.mdfrontend/ios/Runner/Info.plistfrontend/lib/core/di/service_locator.dartfrontend/lib/features/activities/data/activities_context_repository.dartfrontend/lib/screens/home/location_permission_dialog.dartfrontend/lib/services/location_service.dartfrontend/lib/services/service_registry.dartpackages/shared/openapi/openapi-main-weather.json
💤 Files with no reviewable changes (8)
- backend/community-backend/services/community_post_operations.py
- backend/community-backend/services/community_comment_operations.py
- backend/shared/deprecation.py
- backend/tests/test_deprecation_middleware.py
- backend/community-backend/routes/posts.py
- frontend/lib/core/di/service_locator.dart
- frontend/lib/services/service_registry.dart
- backend/shared/contracts.py
…ache leak - Add gt=0 constraints to open_meteo_timeout_seconds, weather_cache_ttl_seconds, and weather_cache_distance_threshold so invalid values are rejected at startup - On cache hit, rebuild weather_snapshot.temperatures with the current request's strava_reported_celsius instead of returning the cached (original) value; all provider-derived fields pass through unchanged via model_copy Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- WeatherLocation: add ge/le bounds on latitude [-90,90] and longitude [-180,180] so invalid coords return 422 before reaching the provider - WeatherService: catch ValueError/JSONDecodeError from response.json() and convert to WeatherServiceError instead of leaking an unhandled 500 - WeatherService._epoch_or_iso_to_utc: attach UTC to naive datetimes before astimezone() so host timezone no longer contaminates the result - README: document the 5 missing WEATHER_CACHE_* environment variables - test_weather: gate live API test behind RUN_INTEGRATION_TESTS env var - query_migration: remove activity_type from deprecated-name mapping; it is the current active filter on the /me route, not a legacy alias - Info.plist: remove 5 duplicate keys (location, camera, photo, indirect-input) that shadowed the correct weather-related copy with an older version - LocationService.hasLocationAccess: consult Permission.location.status alongside Geolocator so the result is consistent with checkPermissions() - openapi-main-weather.json: update example to match Open-Meteo contract (source, condition_code '2' not '802', blank icon_url) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
ServiceRegistry— built Dio clients that were never accessed; GetIt already owns them viaDioFactoryshared/deprecation.py—add_deprecation_middlewarewas never wired to any prod route; only existed in testsshared/query_migration.py159 → 18 lines — onlycheck_deprecated_paramshad a caller; dead functions removedcommunity_post_operations.py/community_comment_operations.py— pure mixin-composition stubs (class X(Read, Write): pass); callers now import the read/write classes directlyroutes/posts.pyaggregator — both post routers now included directly inmain.pyPaginatedResponse/DataResponsealiases incontracts.py— no callers anywhereTest plan
backend/community-backend/tests/test_operations_units.py— updated imports, same test logicpython run.py --service communityand confirm/api/v1/postsand/api/v1/feedstill respondcheck_deprecated_paramsstill works on list endpointsflutter analyzeto confirm no missing imports🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
POST /api/v1/weather/snapshot) and associated weather data models.Bug Fixes
Documentation