Conversation
There was a problem hiding this comment.
Pull request overview
Refactors MapLibre layer handling so GeoJSON layers are treated similarly to other layer types (separate sources/styles per layer) and introduces layer-level flags intended to control clustering/heatmap behavior.
Changes:
- Introduces per-layer GeoJSON sources (
<type>-source-<layerId>) and new feature lookup helpers inmaplibre/layers/layers.js. - Updates various MapLibre modules/controllers to use the new feature lookup approach instead of
geojsonData. - Adds a Wikipedia marker icon and extends
Layersummaries withheatmap/clusterflags.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| public/icons/wikipedia.png | Adds an icon used by the Wikipedia layer features. |
| app/models/layer.rb | Adds heatmap/cluster fields and includes them in serialized layer summaries. |
| app/javascript/maplibre/undo.js | Starts migration away from geojsonData imports (but still needs further refactor). |
| app/javascript/maplibre/styles.js | Updates style initialization and heatmap layer handling; switches feature lookup to getFeature. |
| app/javascript/maplibre/routing/osrm.js | Switches route feature lookup to getFeature. |
| app/javascript/maplibre/map.js | Removes global geojsonData usage patterns and shifts rendering toward per-layer sources. |
| app/javascript/maplibre/layers/layers.js | Initializes per-layer GeoJSON sources/styles and adds getFeature/getFeatures/hasFeatures. |
| app/javascript/maplibre/feature.js | Uses new feature access helpers for edit gating and derived-feature generation inputs. |
| app/javascript/maplibre/edit.js | Uses new helpers for “untouched map” logic and URL-based selection. |
| app/javascript/maplibre/controls/shared.js | Updates feature list items to reference the new per-layer source names. |
| app/javascript/controllers/feature/modal_controller.js | Refactors selected-feature access (now needs getFeature import). |
| app/javascript/controllers/feature/edit_controller.js | Refactors edit-feature access (now needs getFeature import). |
Comments suppressed due to low confidence (1)
app/javascript/maplibre/map.js:123
initializeMaphighlights a URL-selected feature viahighlightFeature(feature, true)without passing the feature’s source. Since the default source inhighlightFeatureisgeojson-source(which is no longer created),setFeatureStatewill fail or highlight the wrong source. Determine the correct source for the feature (e.g., from the owning layer) and pass it explicitly.
const urlFeatureId = new URLSearchParams(window.location.search).get('f')
let feature = getFeature(urlFeatureId)
if (feature) {
resetControls()
highlightFeature(feature, true)
const center = centroid(feature)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // updateData requires a 'GeoJSONSourceDiff', with add/update/remove lists | ||
| map.getSource('geojson-source').setData(renderedGeojsonData()) | ||
| //map.getSource('geojson-source').setData(renderedGeojsonData()) | ||
| console.log('layers:', layers) | ||
| layers.filter(f => f.type !== 'geojson').forEach((layer) => { | ||
| layers.forEach((layer) => { | ||
| if (layer.geojson) { | ||
| console.log("Setting layer data", layer.type, layer.id, layer.geojson) | ||
| map.getSource(layer.type + '-source-' + layer.id).setData(layer.geojson, false) |
There was a problem hiding this comment.
redrawGeojson() no longer calls renderedGeojsonData() and instead pushes layer.geojson directly into each source. This drops the side effects of renderedGeojsonData() (km-marker source updates via renderKmMarkers() and extrusion polygons via renderExtrusionLines()), so km markers / extrusions will stop rendering/updating. Re-introduce those steps in the new per-layer rendering flow (and ensure derived features are included in the appropriate source data).
| const feature = geojsonData.features.find(f => f.id === targetId) | ||
| const feature = getFeature(targetId) | ||
| if (feature) { | ||
| flyToFeature(feature) |
There was a problem hiding this comment.
When handling onclick-target navigation, flyToFeature(feature) is called without a source. flyToFeature defaults to geojson-source, which no longer exists for per-layer sources, so the follow-up highlight will fail. Pass sourceName (or otherwise resolve the owning source) into flyToFeature.
| flyToFeature(feature) | |
| flyToFeature(feature, sourceName) |
| map.addLayer(setSource(styles()[styleName], sourceName)) | ||
| }) | ||
| if (heatmap) { map.addLayer(setSource(styles()['heatmap-layer'], sourceName)) } |
There was a problem hiding this comment.
initializeViewStyles now only adds the heatmap layer when heatmap=true, but existing callers (e.g. overpass layer initialization) invoke it with a single argument. This change will disable rendering for existing heatmap=true overpass queries unless those call sites are updated to pass the flag.
| addGeoJSONSource('geojson-source-' + layer.id, false) | ||
| initializeViewStyles('geojson-source-' + layer.id) |
There was a problem hiding this comment.
GeoJSON layers are always initialized with cluster=false and without passing the new heatmap flag into initializeViewStyles. Since layer summaries now include cluster/heatmap, these settings appear intended to be configurable but are currently ignored. Consider wiring layer.cluster into addGeoJSONSource(...) and layer.heatmap into initializeViewStyles(..., layer.heatmap).
| addGeoJSONSource('geojson-source-' + layer.id, false) | |
| initializeViewStyles('geojson-source-' + layer.id) | |
| addGeoJSONSource('geojson-source-' + layer.id, Boolean(layer.cluster)) | |
| initializeViewStyles('geojson-source-' + layer.id, Boolean(layer.heatmap)) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.