feat: Update disaster types and enhance settings page functionality - #100
Conversation
- Expanded available disaster types in the map page to include wildfire, hurricane, tornado, volcano, and heatwave. - Refactored settings page to replace alert types with watched regions, allowing users to add and manage specific regions for alerts. - Implemented region search functionality to enhance user experience when adding watched regions. - Updated user alert preferences to accommodate the new watched regions feature, improving alert customization.
WalkthroughThis PR redesigns alert preferences from alert_types/regions/disaster_types to watched_regions in the database and API; expands disaster types and enhances analysis with image processing, geocoding, and improved population estimation; updates schema, endpoints, and alert generation logic; adds region search and data feed initialization. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AlertGen as Alert Generator
participant GeoSvc as Geocoding Service
participant DB as Database
participant User as User (Alert Decision)
Client->>DB: Save watched_regions for user
DB-->>Client: Confirmed
loop For each new disaster
AlertGen->>DB: Fetch user alert preferences
DB-->>AlertGen: user_prefs (watched_regions, min_severity)
alt Disaster has coordinates
AlertGen->>AlertGen: Calculate distance to user location
alt Distance ≤ 100 km
AlertGen->>AlertGen: Alert allowed (distance)
else
AlertGen->>GeoSvc: Check if coordinates in watched_regions bounds
GeoSvc-->>AlertGen: Inside bounds? (true/false)
alt Inside region
AlertGen->>AlertGen: Alert allowed (region)
else
AlertGen->>AlertGen: Deny alert
end
end
else No coordinates
alt watched_regions defined
AlertGen->>GeoSvc: Geocode disaster location
GeoSvc-->>AlertGen: Region bounds
AlertGen->>GeoSvc: Check if in watched_regions
GeoSvc-->>AlertGen: Inside bounds? (true/false)
else Global mode
AlertGen->>AlertGen: Alert allowed (global)
end
end
alt Alert allowed
AlertGen->>User: Send alert
else Alert denied
AlertGen->>AlertGen: Skip disaster
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
enhancement
✨ Version Bump PredictionWhen this PR is merged to 1.49.0 → 1.50.0 ( 💡 How to change the version bump typeThe version bump is determined by your commit messages and PR title:
What I analyzed:
Edit your PR title or commit messages to change the bump type. |
github-actions
Bot
added
version: minor
🚀 Preview Deployment Ready!Backend: https://api-feat-geeth-fix-alerts-data-typ.private.bluerelief.app Commit: 🔐 AuthenticationDemo Login: Click "Google Sign In" → Use demo auth (no Google account needed) ✨ Version Bump PredictionWhen this PR is merged to main, the version will be bumped: 1.49.0 → 1.50.0 (minor) 💡 How to change the version bump type
Preview will be automatically deleted when PR is closed or merged. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/tasks.py (1)
320-333: send_alert_emails still depends on removed email_min_severity column
UserAlertPreferences.email_min_severityis dropped in migration2264546da1d9_simplify_alert_preferences.pyand is no longer present on the ORM model (replaced bymin_severity). However, this task still does:# Get user preferences to check email_min_severity user_prefs = ( db.query(UserAlertPreferences) .filter(UserAlertPreferences.user_id == entry.user_id) .first() ) # Check if alert severity meets email threshold if user_prefs and alert.severity < user_prefs.email_min_severity: ... f"... email_min_severity {user_prefs.email_min_severity}"After applying the migration, this will break at runtime (attribute error / ORM column not found) and the severity filter won’t work.
You likely want to use
min_severityas the single source of truth:- # Get user preferences to check email_min_severity + # Get user preferences to check min_severity threshold user_prefs = ( db.query(UserAlertPreferences) .filter(UserAlertPreferences.user_id == entry.user_id) .first() ) - # Check if alert severity meets email threshold - if user_prefs and alert.severity < user_prefs.email_min_severity: + # Check if alert severity meets email threshold + if user_prefs and alert.severity < user_prefs.min_severity: logger.info( - f"Skipping email for alert {alert.id}: severity {alert.severity} < email_min_severity {user_prefs.email_min_severity}" + f"Skipping email for alert {alert.id}: severity {alert.severity} < min_severity {user_prefs.min_severity}" )This keeps the behavior conceptually the same while matching the new schema.
server/services/population_estimator.py (1)
298-312: Critical:backfill_population_estimatescallsestimate_populationwith incompatible signature.The function passes a
Disasterobject, butestimate_populationnow expects individual parameters (longitude,latitude,disaster_type,severity).def backfill_population_estimates(db: Session): """Backfill population estimates for existing disasters without them""" disasters = db.query(Disaster).filter( Disaster.affected_population.is_(None) ).all() updated = 0 for disaster in disasters: - estimate = PopulationEstimator.estimate_population(disaster) + estimate = PopulationEstimator.estimate_population( + latitude=disaster.latitude, + longitude=disaster.longitude, + disaster_type=disaster.disaster_type, + severity=disaster.severity or 3, + ) disaster.affected_population = estimate updated += 1 db.commit() return updated
🧹 Nitpick comments (12)
server/services/geocoding_service.py (1)
62-69: Consider usinglogging.exceptionfor better stack traces.Per static analysis hints, using
logging.exceptioninstead oflogging.errorin exception handlers will automatically include the full stack trace, which is helpful for debugging.except requests.RequestException as e: - logger.error(f"Geocoding request failed: {e}") + logger.exception(f"Geocoding request failed: {e}") return None except (KeyError, TypeError) as e: - logger.error(f"Error parsing geocoding response: {e}") + logger.exception(f"Error parsing geocoding response: {e}") return Noneserver/services/database_service.py (3)
408-421: Remove extraneous f-string prefixes.Per static analysis, lines 413-414 have f-strings without any placeholders. These should be regular strings.
low_effort_patterns = [ f"{disaster_type} in {loc_lower}", f"{disaster_type} reported in", - f"a {disaster_type} occurred", - f"a {disaster_type} occurred in", + f"a {disaster_type} occurred", + f"{disaster_type} occurred in", - f"general mention of", - f"information about", + "general mention of", + "information about", ]
579-579: Use explicitOptionaltype hint.PEP 484 discourages implicit
Optional. Use explicit type annotation for clarity.-def calculate_next_run_time(schedule_hours: int = None) -> datetime: +def calculate_next_run_time(schedule_hours: int | None = None) -> datetime:
645-648: Use bareraiseto preserve stack trace.Using
raisewithout the exception name preserves the full stack trace, which is better for debugging.except Exception as e: db.rollback() print(f"⚠️ Failed to update DataFeed status: {e}") - raise e + raiseserver/routers/data_feed.py (1)
7-8: Data feed status normalization and initialization look correct
ensure_data_feed_initialized()plus the per‑feednext_runcomputation usingcalculate_next_run_time(schedule_hours)produces a clean, client‑friendly status payload:{ "feeds": [{ id, name, status, last_run, next_run }] }The only minor note is that SCHEDULE_HOURS is now read both here and inside the database service; that’s acceptable, but if you ever centralize scheduling config, you might prefer to rely on
calculate_next_run_time()’s default argument instead of reading the env var twice.Also applies to: 21-45
client/app/dashboard/settings/page.tsx (1)
32-43: Watched regions UI and API wiring are aligned and look solid
WatchedRegionmatches the backend model (name,lat,lng, optionalboundsandplace_id).loadPreferences/savePreferencescorrectly read/writewatched_regionsalongsidemin_severityandemail_enabled.searchAndAddRegionintegrates cleanly with/api/alerts/regions/searchand avoids duplicates byplace_id.- Region rendering and
removeRegionhandling are straightforward.One small nuance: if the geocoding service ever returns regions without a
place_id, multiple such regions will be treated as duplicates becauseundefined === undefined. If that becomes an issue, you could fall back tonameor a composite key for deduping.Also applies to: 50-55, 64-76, 85-108, 110-135, 569-627
server/routers/alerts.py (1)
27-33: Watched regions API surface and region search are consistent and well‑wired
WatchedRegionandRegionSearchResultmatch the structure returned bygeocode_regionand expected by the client.UserAlertPreferencesRequest/Responsenow exposewatched_regionsin a way that aligns withUserAlertPreferences.watched_regions(JSON), andupdate_alert_preferencescorrectly serializes Pydantic models viamodel_dump()./regions/searchcleanly wrapsgeocode_region, returning a 404 when no result is found, which is exactly what the UI expects.update_user_locationuses the sameuser_id/lat/lng model as elsewhere and keepsupdated_atin sync.Only minor nit:
WatchedRegionandRegionSearchResultshare the same fields; if this grows more complex, you might factor out a base model to avoid duplication, but it’s fine as-is.Also applies to: 35-52, 54-60, 183-201, 205-237, 249-283, 286-300
server/scripts/backfill_coordinates.py (2)
33-47: Consider reusinggeocode_regionfromgeocoding_service.py.This function duplicates the geocoding logic already available in
server/services/geocoding_service.py. Reusing the existing service would reduce duplication and ensure consistent behavior.+from services.geocoding_service import geocode_region + def geocode_location(query: str) -> dict: """Geocode a location using Google API.""" - try: - url = "https://maps.googleapis.com/maps/api/geocode/json" - params = {"address": query, "key": GOOGLE_API_KEY} - response = requests.get(url, params=params, timeout=10) - response.raise_for_status() - data = response.json() - - if data.get("status") == "OK" and data.get("results"): - location = data["results"][0]["geometry"]["location"] - return {"lat": location["lat"], "lng": location["lng"]} - except Exception as e: - print(f" Geocoding error: {e}") - return None + result = geocode_region(query) + if result: + return {"lat": result["lat"], "lng": result["lng"]} + return None
131-133: Remove extraneousfprefix from strings without placeholders.These f-strings have no interpolated values.
- print(f"\n💾 Changes committed to database") + print("\n💾 Changes committed to database") - print(f"\n📊 Summary:") + print("\n📊 Summary:")server/services/analysis.py (1)
328-343: Consider rate-limiting geocoding calls to avoid API throttling.Calling
geocode_regionin a tight loop for many disasters may hit Google API rate limits. A small delay between calls would be prudent, similar to the rate-limiting inbackfill_coordinates.py.+ import time + for disaster in valid_disasters: location_name = disaster.get("location_name") geo_result = geocode_region(location_name) if geo_result and geo_result.get("lat") and geo_result.get("lng"): disaster["latitude"] = geo_result["lat"] disaster["longitude"] = geo_result["lng"] geocoded_disasters.append(disaster) else: print(f"⚠️ Failed to geocode: {location_name[:50]}") + + # Rate limit to avoid API throttling + if len(valid_disasters) > 10: + time.sleep(0.1)server/services/population_estimator.py (2)
59-64: Non-standard parameter order (longitude before latitude) may cause confusion.The convention is
(lat, lng)in most geospatial APIs. The current signature(longitude, latitude)is counterintuitive and error-prone.def estimate_population( - longitude: float = None, - latitude: float = None, + latitude: float = None, + longitude: float = None, disaster_type: str = None, severity: int = 3, ) -> Optional[int]:Also, per static analysis, use
float | NoneorOptional[float]explicitly:- longitude: float = None, - latitude: float = None, + latitude: Optional[float] = None, + longitude: Optional[float] = None,
140-143: Unused variablecountrydetected by static analysis.The variable is assigned but never used.
elif "country" in types: - country = comp.get("long_name") country_code = comp.get("short_name")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
client/app/dashboard/map/page.tsx(1 hunks)client/app/dashboard/settings/page.tsx(7 hunks)client/app/onboarding/page.tsx(2 hunks)server/alembic/versions/12935d5f34ec_rename_regions_to_watched_regions.py(1 hunks)server/alembic/versions/2264546da1d9_simplify_alert_preferences.py(1 hunks)server/db_utils/db.py(1 hunks)server/requirements.txt(1 hunks)server/routers/alerts.py(5 hunks)server/routers/data_feed.py(2 hunks)server/scripts/backfill_coordinates.py(1 hunks)server/services/alert_generator.py(3 hunks)server/services/analysis.py(3 hunks)server/services/database_service.py(6 hunks)server/services/geocoding_service.py(1 hunks)server/services/population_estimator.py(3 hunks)server/tasks.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/db_utils/db.py
📄 CodeRabbit inference engine (.cursorrules)
Update SQLAlchemy models in
server/db_utils/db.pyFIRST before generating migrations
Files:
server/db_utils/db.py
server/alembic/versions/*.py
📄 CodeRabbit inference engine (.cursorrules)
Use
./scripts/docker-dev.sh migrate-generate "description"to auto-generate migration files, NEVER manually create migration files
Files:
server/alembic/versions/12935d5f34ec_rename_regions_to_watched_regions.pyserver/alembic/versions/2264546da1d9_simplify_alert_preferences.py
🧠 Learnings (2)
📚 Learning: 2025-12-01T19:59:46.772Z
Learnt from: CR
Repo: BlueRelief/bluerelief PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-01T19:59:46.772Z
Learning: Applies to src/**/*.tsx : React component files should use lowercase with dashes naming (e.g., `crisis-map.tsx`, `location-onboarding.tsx`)
Applied to files:
client/app/dashboard/map/page.tsx
📚 Learning: 2025-12-01T19:59:46.772Z
Learnt from: CR
Repo: BlueRelief/bluerelief PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-01T19:59:46.772Z
Learning: Applies to src/**/*.tsx : React component names should use PascalCase (e.g., `CrisisMap`, `LocationOnboarding`)
Applied to files:
client/app/dashboard/map/page.tsx
🧬 Code graph analysis (11)
server/routers/data_feed.py (2)
server/services/database_service.py (2)
ensure_data_feed_initialized(653-689)calculate_next_run_time(579-604)server/db_utils/db.py (1)
DataFeed(205-218)
server/services/analysis.py (1)
server/services/geocoding_service.py (1)
geocode_region(11-69)
server/scripts/backfill_coordinates.py (1)
server/services/archive_service.py (1)
close(190-193)
server/alembic/versions/12935d5f34ec_rename_regions_to_watched_regions.py (1)
server/alembic/versions/2264546da1d9_simplify_alert_preferences.py (2)
upgrade(21-29)downgrade(32-40)
server/services/alert_generator.py (1)
server/services/geocoding_service.py (1)
is_point_in_bounds(72-80)
server/routers/alerts.py (2)
server/services/geocoding_service.py (1)
geocode_region(11-69)server/db_utils/db.py (2)
User(33-54)UserAlertPreferences(270-285)
server/alembic/versions/2264546da1d9_simplify_alert_preferences.py (1)
server/alembic/versions/12935d5f34ec_rename_regions_to_watched_regions.py (2)
upgrade(21-28)downgrade(31-38)
server/services/database_service.py (3)
server/db_utils/db.py (1)
DataFeed(205-218)server/services/population_estimator.py (1)
estimate_population(59-96)server/services/archive_service.py (1)
close(190-193)
client/app/dashboard/settings/page.tsx (3)
server/routers/alerts.py (1)
WatchedRegion(27-32)client/contexts/auth-context.tsx (1)
useAuth(80-86)client/lib/api-client.ts (1)
apiClient(16-43)
server/tasks.py (1)
server/services/database_service.py (2)
get_existing_post_ids(165-181)update_data_feed_status(607-650)
server/services/population_estimator.py (1)
server/db_utils/db.py (1)
Disaster(183-202)
🪛 Ruff (0.14.6)
server/services/analysis.py
51-51: Consider moving this statement to an else block
(TRY300)
52-52: Do not catch blind exception: Exception
(BLE001)
server/services/geocoding_service.py
62-62: Consider moving this statement to an else block
(TRY300)
65-65: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
68-68: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
server/scripts/backfill_coordinates.py
45-45: Do not catch blind exception: Exception
(BLE001)
50-50: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
68-76: Possible SQL injection vector through string-based query construction
(S608)
131-131: f-string without any placeholders
Remove extraneous f prefix
(F541)
133-133: f-string without any placeholders
Remove extraneous f prefix
(F541)
server/routers/alerts.py
218-218: Abstract raise to an inner function
(TRY301)
262-262: Abstract raise to an inner function
(TRY301)
server/services/database_service.py
413-413: f-string without any placeholders
Remove extraneous f prefix
(F541)
414-414: f-string without any placeholders
Remove extraneous f prefix
(F541)
579-579: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
648-648: Use raise without specifying exception name
Remove exception name
(TRY201)
685-685: Do not catch blind exception: Exception
(BLE001)
server/tasks.py
209-209: Do not catch blind exception: Exception
(BLE001)
server/services/population_estimator.py
60-60: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
61-61: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
62-62: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
87-87: Do not catch blind exception: Exception
(BLE001)
142-142: Local variable country is assigned to but never used
Remove assignment to unused variable country
(F841)
169-169: Consider moving this statement to an else block
(TRY300)
171-171: Do not catch blind exception: Exception
(BLE001)
183-183: Local variable url is assigned to but never used
Remove assignment to unused variable url
(F841)
191-191: Local variable resolve_url is assigned to but never used
Remove assignment to unused variable resolve_url
(F841)
192-192: Local variable resolve_params is assigned to but never used
Remove assignment to unused variable resolve_params
(F841)
195-195: Local variable search_url is assigned to but never used
Remove assignment to unused variable search_url
(F841)
205-205: Consider moving this statement to an else block
(TRY300)
207-207: Do not catch blind exception: Exception
(BLE001)
213-213: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
252-252: Consider moving this statement to an else block
(TRY300)
254-254: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (21)
client/app/onboarding/page.tsx (2)
197-204: LGTM - Payload updated for watched_regions model.The API payload correctly initializes
watched_regionstonullduring onboarding, allowing users to configure specific regions later in settings. This aligns with the schema migration fromalert_typestowatched_regions.
441-443: LGTM - Button disabled state simplified correctly.The disabled condition now only checks
savingAlertssincealertTypeswas removed. This is the correct simplification.client/app/dashboard/map/page.tsx (1)
90-90: LGTM - Disaster types expanded and aligned with backend.The updated disaster types now match
VALID_DISASTER_TYPESinserver/services/database_service.py. The changes from "fire" → "wildfire" and "storm" → specific types (hurricane, tornado) improve consistency. Removing "other" in favor of specific types like volcano and heatwave is a good decision for filtering precision.server/services/alert_generator.py (2)
36-37: LGTM - Improved None check for coordinates.Using
any(v is None for v in [...])is more Pythonic and correctly allows0.0coordinates to be treated as valid values.
52-100: Verify edge case: user with location but disaster without coordinates.The refactored logic is clean and well-documented. However, note this edge case: if a user has their location set, but a disaster has no coordinates (
latitude/longitudeare bothNone) and the user has nowatched_regions, the function returnsFalseand the user won't receive the alert.This may be intentional (requiring either distance-based or region-based matching), but worth confirming this is the desired behavior.
server/services/database_service.py (3)
381-430: LGTM - Good quality filtering for disaster data.The pre-filtering logic for coordinates, description length, and low-effort patterns is a sensible addition to improve data quality. The skip counters and logging provide good visibility into filtered items.
607-650: LGTM - Data feed status tracking implementation.The
update_data_feed_statusfunction correctly handles both creation and update cases, increments run count, and calculates next run time. Error handling with rollback is appropriate.
653-689: LGTM - Data feed initialization with defensive handling.The
ensure_data_feed_initializedfunction correctly ensures the feed record exists before tasks run. The silent exception handling (lines 685-688) is acceptable here since this is an initialization helper that shouldn't block startup.server/requirements.txt (1)
96-96: Pillow addition looks fine; confirm runtime supportAdding
Pillow==10.4.0is reasonable for image handling. Just ensure your runtime image has the required system libraries (e.g., libjpeg, zlib) so Pillow can build and handle common formats without runtime errors.server/db_utils/db.py (1)
270-283: UserAlertPreferences watched_regions column aligns with API and migrations
watched_regions = Column(JSON, nullable=True)matches how preferences are exposed via the alerts router and how the Alembic migration adds the column. No issues here.server/alembic/versions/2264546da1d9_simplify_alert_preferences.py (1)
21-39: Schema simplification is fine; ensure all code paths stop using removed columnsDropping
alert_types,email_min_severity, anddisaster_typeshere is consistent with the updated ORM model. Just make sure no remaining code depends on these columns when this migration is applied (notablyemail_min_severityis still referenced inserver/tasks.py::send_alert_emails, which I’ve commented on separately).server/tasks.py (2)
6-13: DISASTER_CONFIG expansion and new imports look consistentThe added imports (
get_existing_post_ids,update_data_feed_status) and the expandedDISASTER_CONFIG(hurricane synonyms, wildfire, tornado, tsunami, volcano, heatwave) are coherent with the broader feature set. No functional issues here.Also applies to: 28-37
107-141: Dedupe-by-URI logic is reasonable; ensure URIs match stored bluesky_idThe new flow that:
- collects all
post.get("uri", "")intoall_post_uris,- uses
get_existing_post_ids(all_post_uris), and- filters
new_postsandnew_disaster_postsby membership inexisting_idsis sound as long as
uriis exactly what you store inPost.bluesky_id. If there’s any mismatch (e.g., different formats or prefixes), deduping will quietly fail.If you’re not 100% sure, it’s worth double‑checking that
fetch_posts’surifield andsave_posts’sbluesky_idare identical.server/scripts/backfill_coordinates.py (1)
139-140: Good practice:finallyblock ensures DB connection cleanup.The connection is properly closed even if an error occurs during processing.
server/services/analysis.py (4)
18-54: Image download implementation looks solid with good error handling.The function correctly handles RGBA/P modes, resizes to save tokens, and gracefully returns
Noneon failure. The timeout prevents hanging on slow image servers.One minor consideration: catching blind
Exceptionis flagged by static analysis. If you want stricter typing, catch(requests.RequestException, IOError, OSError)instead.
236-275: Comprehensive prompt improvements for disaster extraction.The detailed location rules, disaster type mapping, and critical location rules are well-structured and should significantly improve extraction quality. The explicit guidance to skip historical disasters and create separate entries for multi-country events is helpful.
279-287: Multimodal content construction for Gemini API is correctly implemented.The inline_data format with mime_type and base64 data matches the Gemini Vision API requirements.
345-352: Returning empty array when no geocoded disasters is appropriate.Returning
"[]"ensures consistent JSON output and prevents downstream issues with null/undefined handling.server/services/population_estimator.py (3)
17-28: Expanded BASE_ESTIMATES with new disaster types looks good.The estimates for new types (wildfire, tsunami, volcano, heatwave) follow a logical progression and align with the expanded disaster types in the system.
44-56: Type multipliers for impact radius are well-calibrated.Tornados having a small multiplier (0.3) and heatwaves large (3.0) accurately reflects real-world impact patterns.
211-256:_query_world_cities_populationimplementation looks reasonable.The OpenDataSoft API usage is correct, with proper filtering by country code and matching logic for city names. Timeout is set appropriately.
Minor: The broad exception catch could be narrowed to
requests.RequestExceptionfor cleaner error handling, but this is acceptable for a fallback data source.
| def upgrade() -> None: | ||
| """Upgrade schema.""" | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| with op.batch_alter_table('user_alert_preferences', schema=None) as batch_op: | ||
| batch_op.add_column(sa.Column('watched_regions', sa.JSON(), nullable=True)) | ||
| batch_op.drop_column('regions') | ||
|
|
||
| # ### end Alembic commands ### | ||
|
|
||
|
|
||
| def downgrade() -> None: | ||
| """Downgrade schema.""" | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| with op.batch_alter_table('user_alert_preferences', schema=None) as batch_op: | ||
| batch_op.add_column(sa.Column('regions', postgresql.JSON(astext_type=sa.Text()), autoincrement=False, nullable=True)) | ||
| batch_op.drop_column('watched_regions') | ||
|
|
There was a problem hiding this comment.
Renaming regions → watched_regions currently drops all existing region data
The upgrade path:
with op.batch_alter_table('user_alert_preferences', schema=None) as batch_op:
batch_op.add_column(sa.Column('watched_regions', sa.JSON(), nullable=True))
batch_op.drop_column('regions')adds watched_regions but never copies over existing regions values before dropping the old column, so any saved region preferences are lost on migration.
If you care about preserving existing user settings, consider:
def upgrade() -> None:
"""Upgrade schema."""
with op.batch_alter_table('user_alert_preferences', schema=None) as batch_op:
- batch_op.add_column(sa.Column('watched_regions', sa.JSON(), nullable=True))
- batch_op.drop_column('regions')
+ batch_op.add_column(sa.Column('watched_regions', sa.JSON(), nullable=True))
+
+ # Backfill watched_regions from regions before dropping the old column
+ conn = op.get_bind()
+ conn.execute(sa.text(
+ "UPDATE user_alert_preferences "
+ "SET watched_regions = regions "
+ "WHERE watched_regions IS NULL AND regions IS NOT NULL"
+ ))
+
+ with op.batch_alter_table('user_alert_preferences', schema=None) as batch_op:
+ batch_op.drop_column('regions')If you’re intentionally discarding legacy region data, documenting that in the migration docstring/PR description would help future maintainers.
🤖 Prompt for AI Agents
In server/alembic/versions/12935d5f34ec_rename_regions_to_watched_regions.py
around lines 21 to 37, the upgrade currently adds watched_regions then drops
regions, losing existing data; modify the migration to copy existing values
before dropping by running a SQL update (or op.execute) setting watched_regions
= regions for all rows after adding the new column and before dropping the old
one, ensuring JSON type compatibility; likewise update downgrade to add regions,
copy watched_regions back into regions before dropping watched_regions, and
preserve nullable/type metadata and ordering so data is retained across
migrations.
| # Get database URL from env or use default | ||
| DATABASE_URL = os.getenv("DATABASE_URL", "postgresql://dev:devpassword@localhost:5432/bluerelief") | ||
| GOOGLE_API_KEY = os.getenv("GOOGLE_API_KEY") |
There was a problem hiding this comment.
Default credentials in source code pose a security risk.
The default DATABASE_URL contains credentials (dev:devpassword). While likely intended for local development, this can inadvertently be used in non-local contexts if the environment variable is unset.
Consider removing the default to force explicit configuration:
-DATABASE_URL = os.getenv("DATABASE_URL", "postgresql://dev:devpassword@localhost:5432/bluerelief")
+DATABASE_URL = os.getenv("DATABASE_URL")
+if not DATABASE_URL:
+ print("❌ DATABASE_URL environment variable required")
+ sys.exit(1)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/scripts/backfill_coordinates.py around lines 20 to 22, the
DATABASE_URL currently falls back to a hard-coded credentialed default which is
a security risk; remove the default value and require the environment variable
to be set (or alternatively provide a non-credentialed localhost-only sentinel),
and add a clear runtime check that raises an error or exits with a descriptive
message if DATABASE_URL is unset so the process cannot accidentally run with
embedded credentials.
| if limit: | ||
| query = text(f""" | ||
| SELECT id, location_name | ||
| FROM disasters | ||
| WHERE latitude IS NULL | ||
| AND location_name IS NOT NULL | ||
| AND location_name != '' | ||
| ORDER BY id DESC | ||
| LIMIT {limit} | ||
| """) |
There was a problem hiding this comment.
SQL injection vulnerability via string interpolation.
The limit parameter is interpolated directly into the SQL string. While limit comes from argparse as an int, this pattern is unsafe and can break if refactored.
Use parameterized queries:
- if limit:
- query = text(f"""
- SELECT id, location_name
- FROM disasters
- WHERE latitude IS NULL
- AND location_name IS NOT NULL
- AND location_name != ''
- ORDER BY id DESC
- LIMIT {limit}
- """)
+ query = text("""
+ SELECT id, location_name
+ FROM disasters
+ WHERE latitude IS NULL
+ AND location_name IS NOT NULL
+ AND location_name != ''
+ ORDER BY id DESC
+ LIMIT :limit
+ """)
+
+ result = db.execute(query, {"limit": limit or 1000000})Alternatively, apply the limit after fetching if the dataset is small enough.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.6)
68-76: Possible SQL injection vector through string-based query construction
(S608)
🤖 Prompt for AI Agents
In server/scripts/backfill_coordinates.py around lines 67 to 76, the SQL query
interpolates the limit directly into the string creating an SQL injection risk;
instead, change to a parameterized query (use a bind parameter like :limit in
the text() and pass limit as a parameter to execute or use SQLAlchemy's
bindparam) or validate/cast limit to an int before use and pass it as a
parameter to the DB call; ensure you do not perform Python string formatting to
build the SQL and pass the limit via the DB driver's parameter mechanism (or, if
dataset size allows, fetch without LIMIT and slice in Python).
| # Download and include images (max 2 per post, max 10 per batch) | ||
| if image_urls and len(batch_images) < 10: | ||
| for img_url in image_urls[:2]: | ||
| img_data = download_image(img_url) | ||
| if img_data: | ||
| batch_images.append( | ||
| {"post_id": post_id, "post_idx": idx, "image": img_data} | ||
| ) | ||
| line += f"\n [HAS_IMAGE_{len(batch_images)}]" | ||
|
|
||
| posts_lines.append(line) |
There was a problem hiding this comment.
Potential issue: batch_images limit check is off by one.
The condition len(batch_images) < 10 is checked before potentially adding 2 images per post, which could result in up to 11 images.
- if image_urls and len(batch_images) < 10:
- for img_url in image_urls[:2]:
+ if image_urls:
+ for img_url in image_urls[:2]:
+ if len(batch_images) >= 10:
+ break
img_data = download_image(img_url)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Download and include images (max 2 per post, max 10 per batch) | |
| if image_urls and len(batch_images) < 10: | |
| for img_url in image_urls[:2]: | |
| img_data = download_image(img_url) | |
| if img_data: | |
| batch_images.append( | |
| {"post_id": post_id, "post_idx": idx, "image": img_data} | |
| ) | |
| line += f"\n [HAS_IMAGE_{len(batch_images)}]" | |
| posts_lines.append(line) | |
| # Download and include images (max 2 per post, max 10 per batch) | |
| if image_urls: | |
| for img_url in image_urls[:2]: | |
| if len(batch_images) >= 10: | |
| break | |
| img_data = download_image(img_url) | |
| if img_data: | |
| batch_images.append( | |
| {"post_id": post_id, "post_idx": idx, "image": img_data} | |
| ) | |
| line += f"\n [HAS_IMAGE_{len(batch_images)}]" | |
| posts_lines.append(line) |
🤖 Prompt for AI Agents
In server/services/analysis.py around lines 216 to 226, the current check uses
if image_urls and len(batch_images) < 10 before looping and can add up to 2
images per post, allowing 11 images; change the logic to compute remaining_slots
= 10 - len(batch_images) and only attempt to add up to min(2, remaining_slots)
images (or skip if remaining_slots <= 0), and ensure each added image increments
batch_images so the total never exceeds 10 and the [HAS_IMAGE_n] markers align
with actual batch_images length.
| def normalize_disaster_type(disaster_type: str) -> str: | ||
| """Normalize disaster type to one of the 5 standard types.""" |
There was a problem hiding this comment.
Fix docstring: 8 types, not 5.
The docstring says "5 standard types" but VALID_DISASTER_TYPES contains 8 types.
def normalize_disaster_type(disaster_type: str) -> str:
- """Normalize disaster type to one of the 5 standard types."""
+ """Normalize disaster type to one of the 8 standard types."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def normalize_disaster_type(disaster_type: str) -> str: | |
| """Normalize disaster type to one of the 5 standard types.""" | |
| def normalize_disaster_type(disaster_type: str) -> str: | |
| """Normalize disaster type to one of the 8 standard types.""" |
🤖 Prompt for AI Agents
In server/services/database_service.py around lines 53 to 54, the function
docstring incorrectly states "5 standard types" while VALID_DISASTER_TYPES
contains 8 types; update the docstring to reflect the correct number (8) or,
better, describe that it normalizes to the entries in VALID_DISASTER_TYPES
(e.g., "Normalize disaster type to one of the standard types defined in
VALID_DISASTER_TYPES" or "Normalize disaster type to one of the 8 standard
types.") so the docstring matches the actual constants.
| def is_point_in_bounds(lat: float, lng: float, bounds: dict) -> bool: | ||
| """Check if a point falls within region bounds.""" | ||
| if not bounds or lat is None or lng is None: | ||
| return False | ||
|
|
||
| return ( | ||
| bounds["sw_lat"] <= lat <= bounds["ne_lat"] and | ||
| bounds["sw_lng"] <= lng <= bounds["ne_lng"] | ||
| ) |
There was a problem hiding this comment.
Potential issue with antimeridian (longitude 180°/-180°) crossing.
The bounds check assumes simple rectangular bounds, but regions spanning the antimeridian (e.g., Russia, Fiji, parts of Alaska) would have sw_lng > ne_lng. The current logic would incorrectly reject valid points in such regions.
If this edge case matters for your use case, consider:
def is_point_in_bounds(lat: float, lng: float, bounds: dict) -> bool:
"""Check if a point falls within region bounds."""
if not bounds or lat is None or lng is None:
return False
- return (
- bounds["sw_lat"] <= lat <= bounds["ne_lat"] and
- bounds["sw_lng"] <= lng <= bounds["ne_lng"]
- )
+ lat_in_bounds = bounds["sw_lat"] <= lat <= bounds["ne_lat"]
+
+ # Handle antimeridian crossing (sw_lng > ne_lng)
+ if bounds["sw_lng"] <= bounds["ne_lng"]:
+ lng_in_bounds = bounds["sw_lng"] <= lng <= bounds["ne_lng"]
+ else:
+ lng_in_bounds = lng >= bounds["sw_lng"] or lng <= bounds["ne_lng"]
+
+ return lat_in_bounds and lng_in_bounds🤖 Prompt for AI Agents
In server/services/geocoding_service.py around lines 72 to 80, the current
bounds longitude check fails for regions that cross the antimeridian because it
assumes sw_lng <= ne_lng; update the logic to handle two cases: if sw_lng <=
ne_lng keep the existing check, otherwise (antimeridian crossing) treat the
valid longitudes as lng >= sw_lng OR lng <= ne_lng; also normalize input
longitudes and bounds into a consistent range (e.g. -180..180) before comparison
and keep the latitude check unchanged.
| @staticmethod | ||
| def _query_data_commons(city: str, region: str, country_code: str) -> Optional[int]: | ||
| """Query Google Data Commons API for real population data (free, no key needed).""" | ||
| if not city and not region: | ||
| return None | ||
|
|
||
| try: | ||
| # Data Commons API endpoint | ||
| url = "https://api.datacommons.org/v2/observation" | ||
|
|
||
| # Build the place query - try city first, then region | ||
| search_term = f"{city}, {region}" if city else region | ||
| if country_code: | ||
| search_term += f", {country_code}" | ||
|
|
||
| # First, resolve the place name to a DCID | ||
| resolve_url = "https://api.datacommons.org/v2/resolve" | ||
| resolve_params = {"nodes": [search_term], "property": "<-description"} | ||
|
|
||
| # Try a simpler approach - use the node search | ||
| search_url = "https://api.datacommons.org/v2/node" | ||
|
|
||
| # Actually, Data Commons requires specific DCIDs which are hard to get | ||
| # Let's use a simpler population database approach | ||
|
|
||
| # Use OpenDataSoft's world cities population API (free) | ||
| pop = PopulationEstimator._query_world_cities_population(city, country_code) | ||
| if pop: | ||
| return pop | ||
|
|
||
| return None | ||
|
|
||
| except Exception as e: | ||
| print(f"⚠️ Data Commons query failed: {e}") | ||
| return None |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
_query_data_commons contains unused code and just delegates.
This method declares several unused variables (url, resolve_url, resolve_params, search_url) and ultimately only calls _query_world_cities_population. The comment explains the intent was abandoned, but the dead code should be cleaned up.
Either remove this method entirely (it's only called from within itself recursively, which doesn't happen in practice), or clean it up:
@staticmethod
def _query_data_commons(city: str, region: str, country_code: str) -> Optional[int]:
- """Query Google Data Commons API for real population data (free, no key needed)."""
+ """Query population data sources."""
if not city and not region:
return None
- try:
- # Data Commons API endpoint
- url = "https://api.datacommons.org/v2/observation"
-
- # Build the place query - try city first, then region
- search_term = f"{city}, {region}" if city else region
- if country_code:
- search_term += f", {country_code}"
-
- # First, resolve the place name to a DCID
- resolve_url = "https://api.datacommons.org/v2/resolve"
- resolve_params = {"nodes": [search_term], "property": "<-description"}
-
- # Try a simpler approach - use the node search
- search_url = "https://api.datacommons.org/v2/node"
-
- # Actually, Data Commons requires specific DCIDs which are hard to get
- # Let's use a simpler population database approach
-
- # Use OpenDataSoft's world cities population API (free)
- pop = PopulationEstimator._query_world_cities_population(city, country_code)
- if pop:
- return pop
-
- return None
-
- except Exception as e:
- print(f"⚠️ Data Commons query failed: {e}")
- return None
+ return PopulationEstimator._query_world_cities_population(city, country_code)Note: This method doesn't appear to be called anywhere in the current code - consider removing it entirely.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @staticmethod | |
| def _query_data_commons(city: str, region: str, country_code: str) -> Optional[int]: | |
| """Query Google Data Commons API for real population data (free, no key needed).""" | |
| if not city and not region: | |
| return None | |
| try: | |
| # Data Commons API endpoint | |
| url = "https://api.datacommons.org/v2/observation" | |
| # Build the place query - try city first, then region | |
| search_term = f"{city}, {region}" if city else region | |
| if country_code: | |
| search_term += f", {country_code}" | |
| # First, resolve the place name to a DCID | |
| resolve_url = "https://api.datacommons.org/v2/resolve" | |
| resolve_params = {"nodes": [search_term], "property": "<-description"} | |
| # Try a simpler approach - use the node search | |
| search_url = "https://api.datacommons.org/v2/node" | |
| # Actually, Data Commons requires specific DCIDs which are hard to get | |
| # Let's use a simpler population database approach | |
| # Use OpenDataSoft's world cities population API (free) | |
| pop = PopulationEstimator._query_world_cities_population(city, country_code) | |
| if pop: | |
| return pop | |
| return None | |
| except Exception as e: | |
| print(f"⚠️ Data Commons query failed: {e}") | |
| return None | |
| @staticmethod | |
| def _query_data_commons(city: str, region: str, country_code: str) -> Optional[int]: | |
| """Query population data sources.""" | |
| if not city and not region: | |
| return None | |
| return PopulationEstimator._query_world_cities_population(city, country_code) |
🧰 Tools
🪛 Ruff (0.14.6)
183-183: Local variable url is assigned to but never used
Remove assignment to unused variable url
(F841)
191-191: Local variable resolve_url is assigned to but never used
Remove assignment to unused variable resolve_url
(F841)
192-192: Local variable resolve_params is assigned to but never used
Remove assignment to unused variable resolve_params
(F841)
195-195: Local variable search_url is assigned to but never used
Remove assignment to unused variable search_url
(F841)
205-205: Consider moving this statement to an else block
(TRY300)
207-207: Do not catch blind exception: Exception
(BLE001)
| complete_collection_run(run.id, total_saved, "completed") | ||
|
|
||
| update_data_feed_status() | ||
|
|
||
| print(f"\n[{datetime.now()}] Multi-Disaster job completed successfully!") |
There was a problem hiding this comment.
Avoid letting DataFeed status updates mark successful runs as failed
Right now update_data_feed_status() is inside the main try block. If it throws on the success path (after collection and analysis succeed), the exception bubbles up, and the except block:
- calls
complete_collection_run(run.id, 0, "failed", error_msg)— overwriting the realtotal_savedand status, and - then calls
update_data_feed_status()again in a nestedtry, which may also fail but is swallowed.
That means a transient failure updating the data_feeds table can flip an otherwise successful job into a “failed” run with incorrect metrics.
I’d treat the DataFeed update as best‑effort and decouple it from the main task failure state:
156 complete_collection_run(run.id, total_saved, "completed")
157-
158- update_data_feed_status()
159-
+ # Best-effort: don't let DataFeed status failures break the whole job
+ try:
+ update_data_feed_status()
+ except Exception as update_error:
+ print(f"⚠️ Failed to update feed status: {update_error}")
+
160 print(f"\n[{datetime.now()}] Multi-Disaster job completed successfully!")
...
203 return results
204 except Exception as e:
205 error_msg = f"Error in multi-disaster collection: {str(e)}"
206 complete_collection_run(run.id, 0, "failed", error_msg)
207- try:
208- update_data_feed_status()
209- except Exception as update_error:
210- print(f"⚠️ Failed to update feed status: {update_error}")
211 print(f"\n[{datetime.now()}] {error_msg}")This way:
- collection_run status reflects the actual task outcome, and
- DataFeed tracking is still updated when possible, but won’t corrupt run metrics on failure.
Also applies to: 205-211
🤖 Prompt for AI Agents
In server/tasks.py around lines 156-160 (and similarly for 205-211),
update_data_feed_status() is currently called inside the main try so any error
there can make a successful run be marked as failed and overwrite real metrics;
make the DataFeed update best-effort by moving update_data_feed_status() out of
the main try/except or wrapping it in its own try/except that only logs errors
and does not call complete_collection_run() or alter the real
total_saved/status; ensure complete_collection_run(run.id, total_saved,
"completed") is always called on success before attempting the non-blocking data
feed update, and on failure only the actual failure path sets run to failed—do
not let DataFeed update exceptions change run metrics or status.
2 participants
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.