fix: POI filter matches activities and unifies icon taxonomy (#410)#423
Conversation
The POI panel filter and map legend chips now search activities, not just icon type. Typing "camping" or clicking the Camping chip shows all POIs with camping as an activity, regardless of their assigned icon type. - Add poiMatchesActivityForTypes() utility to match POI activities against enabled icon types' activity_fallbacks - Update Map.jsx visibility checks (MapBoundsTracker + marker rendering) and zoom-to-fit bounds to include activity-matched POIs - Extend text search in App.jsx and ResultsTab.jsx to match primary_activities - Add 4 new icon types: Fishing, Kayaking, Scenic, Art & Culture with SVGs - Update Nature icon fallbacks to include Bird Watching and Photography - Rename Visitor Center to Discovery (covers museums, libraries, visitor centers) - Remove museum from Historic keywords (Discovery claims it at higher priority) - Fix orphaned POIs: Gear Up Velo → biking, John Brown Monument → historic - Soft-delete duplicate Brecksville Reservation point POI (boundary exists) - Disable Other/default icon type — all point POIs now map to real types - Increase legend panel height from 480px to 580px for additional icon types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a database migration to unify activities and icons, adding new icon types (fishing, kayaking, scenic, art) with corresponding SVG files, updating fallbacks, and syncing the activities table. It also updates the frontend to support searching and filtering POIs by name or activity, and adjusts map bounds tracking. The review feedback highlights a correctness discrepancy in map bounds calculation, warns against hardcoding specific database IDs in SQL migrations, and suggests caching split fallbacks in poiMatchesActivityForTypes to improve performance.
| const poiActs = (dest.primary_activities || '').toLowerCase(); | ||
| if (poiActs && iconConfig) { | ||
| for (const icon of iconConfig) { | ||
| if (icon.enabled === false || !icon.activity_fallbacks || icon.name === t) continue; | ||
| const fbs = icon.activity_fallbacks.split(',').map(a => a.trim().toLowerCase()); | ||
| if (fbs.some(fb => fb && poiActs.includes(fb))) { | ||
| addToBounds(icon.name, lat, lng); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a correctness discrepancy between how typeBoundsById matches activities (using substring matching via includes) and how poiMatchesActivityForTypes matches them (using whole-word matching via matchesWholeWord). This can cause typeBoundsById to include a POI in the bounds for an icon type even though that POI is not actually rendered on the map for that type, leading to incorrect map zoom/bounds fitting. We can fix this and improve performance by reusing poiMatchesActivityForTypes with a single reused Set instance.
const poiActs = (dest.primary_activities || '').toLowerCase();
if (poiActs && iconConfig) {
const tempSet = new Set();
for (const icon of iconConfig) {
if (icon.enabled === false || !icon.activity_fallbacks || icon.name === t) continue;
tempSet.clear();
tempSet.add(icon.name);
if (poiMatchesActivityForTypes(dest, tempSet, iconConfig)) {
addToBounds(icon.name, lat, lng);
}
}
}| UPDATE pois SET primary_activities = 'Photography,Historical Tours' WHERE name = 'Quaker Square' AND primary_activities = 'Photography'; | ||
|
|
||
| -- Brecksville Reservation point POI → soft-delete (boundary version exists) | ||
| UPDATE pois SET deleted = true WHERE id = 5745 AND name = 'Brecksville Reservation' AND 'point' = ANY(poi_roles); |
There was a problem hiding this comment.
Hardcoding specific database IDs (like 5745) in migrations is risky because IDs can differ across environments (development, staging, production) due to different insertion orders or manual edits. Since you are already filtering by name and poi_roles, you can safely remove the hardcoded id filter to make the migration robust across all environments.
UPDATE pois SET deleted = true WHERE name = 'Brecksville Reservation' AND 'point' = ANY(poi_roles);| export function poiMatchesActivityForTypes(poi, visibleTypes, iconConfig) { | ||
| if (!iconConfig || iconConfig.length === 0) return false; | ||
| const poiActivities = (poi.primary_activities || '').toLowerCase(); | ||
| if (!poiActivities) return false; | ||
|
|
||
| for (const icon of iconConfig) { | ||
| if (icon.enabled === false) continue; | ||
| if (!visibleTypes.has(icon.name)) continue; | ||
| if (!icon.activity_fallbacks) continue; | ||
|
|
||
| const fallbacks = icon.activity_fallbacks.split(',').map(a => a.trim().toLowerCase()); | ||
| for (const fb of fallbacks) { | ||
| if (fb && matchesWholeWord(poiActivities, fb)) return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The poiMatchesActivityForTypes function is called repeatedly for many POIs during rendering and map movement. Splitting and mapping icon.activity_fallbacks on every single call is highly inefficient. We can cache the split and normalized fallbacks using a WeakMap on globalThis to avoid redundant string operations and significantly improve performance.
export function poiMatchesActivityForTypes(poi, visibleTypes, iconConfig) {
if (!iconConfig || iconConfig.length === 0) return false;
const poiActivities = (poi.primary_activities || '').toLowerCase();
if (!poiActivities) return false;
if (!globalThis._iconFallbackCache) {
globalThis._iconFallbackCache = new WeakMap();
}
const cache = globalThis._iconFallbackCache;
for (const icon of iconConfig) {
if (icon.enabled === false) continue;
if (!visibleTypes.has(icon.name)) continue;
if (!icon.activity_fallbacks) continue;
let fallbacks = cache.get(icon);
if (!fallbacks) {
fallbacks = icon.activity_fallbacks.split(',').map(a => a.trim().toLowerCase());
cache.set(icon, fallbacks);
}
for (const fb of fallbacks) {
if (fb && matchesWholeWord(poiActivities, fb)) return true;
}
}
return false;
}Gatehouse review found typeBoundsById used substring includes() while poiMatchesActivityForTypes used whole-word regex matching. Standardize on matchesWholeWord and export it from iconUtils.js. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Closes #410
Test plan
🤖 Generated with Claude Code