Skip to content

feat: geofenced & associated news and events rollup (#406)#424

Merged
fatherlinux merged 5 commits into
masterfrom
feature/406-geofenced-news
May 27, 2026
Merged

feat: geofenced & associated news and events rollup (#406)#424
fatherlinux merged 5 commits into
masterfrom
feature/406-geofenced-news

Conversation

@fatherlinux
Copy link
Copy Markdown
Member

Summary

Clicking a geographic boundary or an organization in the sidebar now rolls up News & Events for everything that boundary contains or that org owns — not just content collected against the single POI.

  • Boundary (e.g. Brecksville Reservation) → its own content + every POI whose location is inside the polygon (ST_Contains).
  • Organization (e.g. Cleveland Metroparks) → its own content + owned (owner_id) / associated (poi_associations) POIs + POIs geographically inside any owned park boundary.
  • Plain point POI → unchanged (id set is just [id], no spatial cost).

How

  • backend/services/geoService.js: new getRollupPoiIds(pool, poiId). Spatial steps wrapped in try/catch → degrade to the non-spatial expansion on PostGIS failure, mirroring getContainingBoundaries.
  • backend/server.js: /api/pois/:id/{news,events,tab-counts} query poi_id = ANY($ids) over the rollup set and return each item's poi_id + source poi_name.
  • frontend/src/components/sidebar/PoiNews.jsx, PoiEvents.jsx: label rolled-up items with their source POI (📍) and build the permalink slug from the item's source POI so click-through opens the right detail page.

No schema changes, no migrations, no new endpoints. Spec/plan in .specify/specs/026-geofenced-news/.

Closes #406

Test plan

  • ./run.sh build passes
  • Boundary rollup: CVNP → 203 news from contained POIs, each source-labeled
  • Org rollup: Cleveland Metroparks → 66 news from owned parks + contained POIs
  • Regression: Blossom Music Center (point) → 7 own-only news, single poi_id
  • gourmand --full . clean
  • Human verification in browser

🤖 Generated with Claude Code

fatherlinux and others added 2 commits May 27, 2026 03:33
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clicking a boundary (e.g. Brecksville Reservation) or an organization
(e.g. Cleveland Metroparks) now shows News & Events for everything the
boundary contains or the org owns, not just content collected against
that single POI.

- backend: getRollupPoiIds() expands a POI id into the set whose
  news/events roll up to it — contained POIs (ST_Contains) for a
  boundary, owned/associated POIs plus POIs inside owned park
  boundaries for an organization, just [id] for a plain point.
  Spatial steps degrade gracefully to the non-spatial set on PostGIS
  failure, mirroring getContainingBoundaries.
- backend: /api/pois/:id/{news,events,tab-counts} query poi_id = ANY()
  over the rollup set and return each item's source poi_id + poi_name.
- frontend: PoiNews/PoiEvents label rolled-up items with their source
  POI and link them to that POI's own permalink.

Closes #406.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements geofenced and associated news and events rollup functionality for boundary and organization POIs. It introduces a helper function getRollupPoiIds to expand target POI IDs and updates endpoints and frontend components to support this rollup. The feedback identifies two key issues: first, a PostGIS query issue in geoService.js where ST_GeometryN will return NULL for simple LINESTRING geometries unless wrapped in ST_Multi; second, a lack of input validation for the id parameter in the /api/pois/:id/news and /api/pois/:id/events endpoints.

Comment thread backend/services/geoService.js Outdated
Comment on lines +128 to +129
WHEN p.poi_roles && ARRAY['trail','boundary','river']::text[] AND p.geometry IS NOT NULL THEN
ST_StartPoint(ST_GeometryN(ST_GeomFromGeoJSON(p.geometry::text), 1))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of ST_GeometryN directly on the parsed GeoJSON geometry will return NULL if the geometry is a simple LINESTRING (which is common for custom or imported trails/rivers), as ST_GeometryN is only defined for multi-geometries or collections in PostGIS. Consequently, ST_StartPoint will receive NULL and return NULL, causing simple linear features to be silently excluded from the spatial rollup.

Wrapping the geometry in ST_Multi first guarantees it is treated as a multi-geometry, making it compatible with ST_GeometryN regardless of whether the underlying geometry is a simple or multi-line string.

Suggested change
WHEN p.poi_roles && ARRAY['trail','boundary','river']::text[] AND p.geometry IS NOT NULL THEN
ST_StartPoint(ST_GeometryN(ST_GeomFromGeoJSON(p.geometry::text), 1))
WHEN p.poi_roles && ARRAY['trail','boundary','river']::text[] AND p.geometry IS NOT NULL THEN
ST_StartPoint(ST_GeometryN(ST_Multi(ST_GeomFromGeoJSON(p.geometry::text)), 1))

Comment thread backend/server.js
Comment on lines 1924 to +1928
try {
const { id } = req.params;
const limit = parseInt(req.query.limit) || 50;
// Roll up boundary/org POIs to include contained/owned POIs (#406)
const poiIds = await getRollupPoiIds(pool, id);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The id parameter from req.params is not validated before being passed to getRollupPoiIds. If a non-integer or invalid ID is provided, it will propagate to the database query and could result in unnecessary database roundtrips or errors.

Adding an upfront validation check (similar to the one implemented in /api/pois/:id/tab-counts) ensures consistency and improves defensive programming.

Note: The same validation should also be applied to the /api/pois/:id/events endpoint.

  try {
    const id = parseInt(req.params.id, 10);
    if (!Number.isInteger(id) || id <= 0) {
      return res.status(400).json({ error: 'Invalid POI id' });
    }
    const limit = parseInt(req.query.limit) || 50;
    // Roll up boundary/org POIs to include contained/owned POIs (#406)
    const poiIds = await getRollupPoiIds(pool, id);

fatherlinux and others added 3 commits May 27, 2026 03:54
…review)

- Replace the all-POIs candidate CTE with two paths: point POIs join
  directly on the indexed geom column (uses idx_pois_geom GiST index),
  linear features parse GeoJSON only for the small trail/river/boundary
  subset. CVNP containment now ~2ms via index scan.
- getRollupPoiIds returns [] for a non-existent POI, consistent with
  the invalid-input path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iew)

ST_StartPoint returns NULL for polygon-type geometries, so boundary-role
POIs contained within a larger boundary were silently dropped from the
rollup. ST_PointOnSurface returns a guaranteed-interior representative
point for LineString, MultiLineString, Polygon, and MultiPolygon alike —
verified against all four geometry types present in the data.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pois.geometry is a jsonb column; ST_GeomFromGeoJSON accepts jsonb
directly on PostGIS 3.5, so the ::text cast forced a needless
jsonb->text round-trip. Drop the cast.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@fatherlinux
Copy link
Copy Markdown
Member Author

Gatehouse review disposition

Fixed (4 commits):

  • HIGH (perf): replaced the all-POIs candidate CTE with two paths — point POIs join directly on the indexed geom column (idx_pois_geom GiST; EXPLAIN ANALYZE confirms an index scan, CVNP containment ~2 ms), and GeoJSON parsing is limited to the small trail/river/boundary subset.
  • HIGH (bug): ST_StartPoint returns NULL for polygon-type geometries, silently dropping boundary-role contained features. Switched to ST_PointOnSurface, which returns a guaranteed-interior point for LineString/MultiLineString/Polygon/MultiPolygon (verified against all four types in the data).
  • MEDIUM (bug): getRollupPoiIds now returns [] for a non-existent POI, consistent with the invalid-input path.
  • CRITICAL→micro (perf): dropped the ::text cast — pois.geometry is jsonb and PostGIS 3.5 ST_GeomFromGeoJSON accepts jsonb directly, avoiding a round-trip.

Justified (no change):

  • XSS findings — false positive. All values render as JSX text children ({item.foo}), which React auto-escapes; there is no dangerouslySetInnerHTML. title/summary/description rendering is pre-existing.
  • HIGH: migrate geometry jsonb → native PostGIS column — out of scope; that column is app-wide infrastructure. The parse cost is bounded to ~245 linear-feature rows and is not the hot path (point rollup is index-backed).
  • MEDIUM: getRollupPoiIds called 3× — these are three separate endpoints/HTTP requests, each calling it once; every call is index-backed (~2 ms). A cross-request cache adds invalidation complexity not warranted at this scale.
  • 2× LOW: snake_case item.poi_name/item.poi_id — matches the established API-field convention in these components (item.news_type, item.start_date, …); camelCasing only these would be inconsistent.

@fatherlinux fatherlinux merged commit b660ee0 into master May 27, 2026
3 checks passed
@fatherlinux fatherlinux deleted the feature/406-geofenced-news branch May 27, 2026 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: GeoFenced and Associated News

1 participant