Skip to content

Conversation

@digitaltom
Copy link
Collaborator

No description provided.

Copilot AI review requested due to automatic review settings February 2, 2026 23:34
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 in maplibre/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 Layer summaries with heatmap/cluster flags.

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

  • initializeMap highlights a URL-selected feature via highlightFeature(feature, true) without passing the feature’s source. Since the default source in highlightFeature is geojson-source (which is no longer created), setFeatureState will 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.

Comment on lines 22 to 25
def to_summary_json
json = { id: id, type: type, name: name }
json = { id: id, type: type, name: name, heatmap: !!heatmap, cluster: !!cluster }
json[:query] = query if type == "overpass"
json
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

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

to_summary_json now includes heatmap/cluster, but broadcast_update only broadcasts when name or query change. If heatmap/cluster are updated, connected clients won’t receive an update_layer event and will render stale settings; include these fields in the change check (or broadcast on any relevant update).

Copilot uses AI. Check for mistakes.
Comment on lines 407 to 413
// 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)
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.
const feature = geojsonData.features.find(f => f.id === targetId)
const feature = getFeature(targetId)
if (feature) {
flyToFeature(feature)
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
flyToFeature(feature)
flyToFeature(feature, sourceName)

Copilot uses AI. Check for mistakes.
Comment on lines 34 to +37
map.addLayer(setSource(styles()[styleName], sourceName))
})
if (heatmap) { map.addLayer(setSource(styles()['heatmap-layer'], sourceName)) }
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
f.e('#edit-buttons button', (e) => { e.classList.remove('active') })
// allow edit in rw mode for geojson features only
if (window.gon.map_mode === 'rw' && geojsonData.features.find(f => f.id === feature.id)) {
if (window.gon.map_mode === 'rw' && getFeature(feature.id)) {
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

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

The edit-mode gate uses getFeature(feature.id), but getFeature currently searches all layer types (geojson/overpass/wikipedia). That means non-geojson features can incorrectly be treated as editable and show the edit UI in rw mode. Restrict this check to geojson layers only (e.g., getFeature(feature.id, 'geojson') or a dedicated getGeojsonFeature).

Suggested change
if (window.gon.map_mode === 'rw' && getFeature(feature.id)) {
if (window.gon.map_mode === 'rw' && getFeature(feature.id, 'geojson')) {

Copilot uses AI. Check for mistakes.
Comment on lines 18 to 22
initLayers.forEach((layer) => {
addGeoJSONSource('geojson-source-' + layer.id, false)
initializeViewStyles('geojson-source-' + layer.id)
})
redrawGeojson()
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

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

initializeLayers() calls redrawGeojson() before initializeOverpassLayers() / initializeWikipediaLayers(). If an overpass/wikipedia layer already has cached/preloaded layer.geojson, redrawGeojson() will attempt map.getSource('<type>-source-<id>').setData(...) before those sources are created and will throw. Create sources for all layers before calling redrawGeojson(), or guard redrawGeojson() against missing sources.

Copilot uses AI. Check for mistakes.
Comment on lines 85 to 86
const feature = getFeature(urlFeatureId)
if (feature) { map.fire('draw.selectionchange', {features: [feature]}) }
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

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

On geojson.load, this selects urlFeatureId via getFeature(urlFeatureId) and then fires a draw selectionchange. Since getFeature currently searches all layer types, a non-geojson feature id in the URL could be returned and passed into draw, which expects editable geojson features. Restrict this lookup to geojson layers only (or use a geojson-specific helper).

Suggested change
const feature = getFeature(urlFeatureId)
if (feature) { map.fire('draw.selectionchange', {features: [feature]}) }
if (!urlFeatureId) { return }
const feature = getFeature(urlFeatureId)
if (feature && feature.layer && feature.layer.type === 'geojson') {
map.fire('draw.selectionchange', { features: [feature] })
}

Copilot uses AI. Check for mistakes.
Comment on lines 19 to 20
addGeoJSONSource('geojson-source-' + layer.id, false)
initializeViewStyles('geojson-source-' + layer.id)
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
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))

Copilot uses AI. Check for mistakes.
Comment on lines 407 to 409
// 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)
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

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

The commented-out map.getSource('geojson-source').setData(renderedGeojsonData()) suggests renderedGeojsonData() might be re-enabled later, but that function still references geojsonData (now removed). Either update renderedGeojsonData() to use the new per-layer/merged storage or delete it to avoid a latent ReferenceError if this line is uncommented.

Copilot uses AI. Check for mistakes.
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.

2 participants