Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ Implements MapLibre's `IControl`. The control renders as a 29x29 toggle button t
| Method | Description |
| --- | --- |
| `addServiceLayer(service, sublayerId?, label?)` | Adds a service or single MapServer sublayer to the map (async; resolves with the added layer) |
| `restoreLayers(entries)` | Re-registers host-persisted `AddedLayer` entries, reusing native sources/layers the host already recreated (no fitBounds, no notices). Deferred until `onAdd` when the control is not yet on a map |
| `removeLayer(id)` | Removes a layer added through the control |
| `setLayerOpacity(id, opacity)` | Sets the raster opacity of an added layer |
| `setLayerVisibility(id, visible)` | Shows or hides an added layer |
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "maplibre-gl-enviroatlas",
"version": "0.1.0",
"version": "0.1.1",
"description": "A MapLibre GL JS plugin for searching and adding EPA EnviroAtlas web services to a map",
"type": "module",
"main": "./dist/index.cjs",
Expand Down
52 changes: 52 additions & 0 deletions src/lib/core/EnviroAtlasControl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ export class EnviroAtlasControl implements IControl {
private _searchEpoch = 0;
private _noticeTimer: ReturnType<typeof setTimeout> | null = null;
private _debouncedSearch?: (query: string) => void;
/** Layers handed to restoreLayers before the control was added to a map */
private _pendingRestore: AddedLayer[] = [];

// Panel positioning handlers
private _resizeHandler: (() => void) | null = null;
Expand Down Expand Up @@ -155,6 +157,14 @@ export class EnviroAtlasControl implements IControl {
// Setup event listeners for panel positioning and click-outside
this._setupEventListeners();

// Apply any layers handed to restoreLayers before the control was
// added to a map.
if (this._pendingRestore.length > 0) {
const pending = this._pendingRestore;
this._pendingRestore = [];
this.restoreLayers(pending);
}

// Set initial panel state
if (!this._state.collapsed) {
this._panel.classList.add('expanded');
Expand Down Expand Up @@ -207,6 +217,7 @@ export class EnviroAtlasControl implements IControl {
this._allServices = [];
this._prefetchStarted = false;
this._state.addedLayers = [];
this._pendingRestore = [];

// Remove panel from map container
this._panel?.parentNode?.removeChild(this._panel);
Expand Down Expand Up @@ -371,6 +382,47 @@ export class EnviroAtlasControl implements IControl {
}
}

/**
* Re-registers layers that were previously added and persisted by a
* host application.
*
* Host applications that save and restore map state (for example a
* project file) typically recreate the native MapLibre source and
* layer for each persisted {@link AddedLayer} themselves and then need
* to hand those layers back to the control so it tracks them again.
* This method does exactly that without duplicating the natives: when
* the source/layer already exist they are reused (and opacity and
* visibility reconciled), otherwise they are created.
*
* Entries that are already tracked, or that match an existing layer by
* service and sublayer, are skipped. Unlike {@link addServiceLayer},
* this method never fits the map bounds and shows no notices.
*
* When the control has not been added to a map yet the entries are
* stored and applied automatically in {@link onAdd}.
*
* @param entries - The persisted added-layer entries to restore
*/
restoreLayers(entries: AddedLayer[]): void {
if (!this._layerManager) {
this._pendingRestore.push(...entries);
return;
}

let restored = false;
for (const entry of entries) {
if (this._layerManager.findLayer(entry.service, entry.sublayerId)) continue;
const layer = this._layerManager.restoreLayer(entry);
restored = true;
this._emit('layeradd', { layer });
}
Comment on lines +413 to +418
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Skip already-tracked IDs before emitting layeradd.

This loop only de-dupes by service+sublayer. If an entry is already tracked by id but not by that key, restoreLayer can return the existing layer and this still emits a new layeradd, violating the “already tracked entries are skipped” contract.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/core/EnviroAtlasControl.ts` around lines 413 - 418, Loop currently
only checks service+sublayer and can still emit layeradd when an entry with the
same id is already tracked; before calling
this._layerManager.restoreLayer(entry) (or before emitting), call the layer-id
check using the entry.id (e.g., this._layerManager.findLayer(entry.id)) and skip
this entry if a layer with that id already exists so you don't emit a duplicate
this._emit('layeradd', { layer }) for already-tracked ids.


if (!restored) return;
this._state.addedLayers = this._layerManager.getLayers();
this._addedView?.update(this._state.addedLayers);
this._emit('statechange');
Comment on lines +413 to +423
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Isolate per-entry restore failures so one bad entry doesn’t break batch/onAdd.

restoreLayer exceptions currently bubble out of restoreLayers. Because onAdd calls this method directly, one malformed/conflicting entry can abort control initialization and leave partial restores without final state/UI reconciliation.

Suggested fix
   restoreLayers(entries: AddedLayer[]): void {
     if (!this._layerManager) {
       this._pendingRestore.push(...entries);
       return;
     }

+    const tracked = this._layerManager.getLayers();
+    const seenIds = new Set(tracked.map((l) => l.id));
+    const seenServiceKeys = new Set(
+      tracked.map((l) => `${l.service.fullName}::${l.sublayerId ?? ''}`)
+    );
+
     let restored = false;
     for (const entry of entries) {
-      if (this._layerManager.findLayer(entry.service, entry.sublayerId)) continue;
-      const layer = this._layerManager.restoreLayer(entry);
-      restored = true;
-      this._emit('layeradd', { layer });
+      const serviceKey = `${entry.service.fullName}::${entry.sublayerId ?? ''}`;
+      if (seenIds.has(entry.id) || seenServiceKeys.has(serviceKey)) continue;
+      try {
+        const layer = this._layerManager.restoreLayer(entry);
+        restored = true;
+        seenIds.add(layer.id);
+        seenServiceKeys.add(`${layer.service.fullName}::${layer.sublayerId ?? ''}`);
+        this._emit('layeradd', { layer });
+      } catch (error) {
+        this._handleError(error instanceof Error ? error : new Error(String(error)));
+      }
     }

     if (!restored) return;
     this._state.addedLayers = this._layerManager.getLayers();
     this._addedView?.update(this._state.addedLayers);
     this._emit('statechange');
   }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/core/EnviroAtlasControl.ts` around lines 413 - 423, The loop that
calls this._layerManager.restoreLayer(entry) should isolate per-entry failures
so one thrown exception doesn't abort the whole batch: wrap the call to
restoreLayer(entry) in a try/catch, on success set restored = true, call
this._emit('layeradd', { layer }) and continue; on catch log or handle the
single-entry error (do not rethrow) and continue with the next entry. After the
loop keep the existing final block that sets this._state.addedLayers =
this._layerManager.getLayers(), calls
this._addedView?.update(this._state.addedLayers) and this._emit('statechange')
only if restored is true so partial successes still reconcile UI/state. Ensure
references: restoreLayer, _layerManager, _emit, _state, and _addedView are used
as shown.

}

/**
* Removes a layer previously added through the control.
*
Expand Down
78 changes: 70 additions & 8 deletions src/lib/core/mapLayers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,63 @@ export class MapLayerManager {
bounds,
};

this._createSource(entry);
this._createLayer(entry);

this._layers.set(id, entry);
return entry;
}

/**
* Re-registers a previously persisted layer, reusing native source
* and layer objects the host application may have already recreated.
*
* Host applications that persist {@link AddedLayer} entries (for
* example in a saved project) often recreate the native MapLibre
* source and layer themselves before re-activating the control. This
* method hands those layers back to the manager without duplicating
* the natives: existing source/layer objects are kept and only the
* missing ones are created, while opacity and visibility are
* reconciled to match the entry.
*
* @param entry - The persisted added-layer entry to restore
* @returns The tracked added-layer entry (an existing one when the id
* was already managed, otherwise the newly registered copy)
*/
restoreLayer(entry: AddedLayer): AddedLayer {
const existing = this._layers.get(entry.id);
if (existing) return existing;

const copy: AddedLayer = { ...entry };

if (!this._map.getSource(copy.sourceId)) {
this._createSource(copy);
} else if (this._options.renderMode === 'image') {
// The host recreated the source; still ensure the shared view
// listener that refreshes image layers is registered.
this._ensureViewHandler();
}

if (!this._map.getLayer(copy.layerId)) {
this._createLayer(copy);
} else {
Comment on lines +154 to +156
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Do not delete pre-existing sources on restore-layer failure.

When restoreLayer hits this branch, copy.sourceId may belong to a host-recreated source. If _createLayer throws, _createLayer’s catch currently removes that source unconditionally, which can delete a native source the manager did not create.

Suggested fix
-  private _createLayer(entry: AddedLayer): void {
+  private _createLayer(entry: AddedLayer, removeSourceOnFailure = true): void {
@@
     } catch (error) {
       // Avoid orphaning the source when layer creation fails
-      if (this._map.getSource(entry.sourceId)) {
+      if (removeSourceOnFailure && this._map.getSource(entry.sourceId)) {
         this._map.removeSource(entry.sourceId);
       }
       throw error;
     }
   }
-    if (!this._map.getLayer(copy.layerId)) {
-      this._createLayer(copy);
+    const sourceExisted = Boolean(this._map.getSource(copy.sourceId));
+    if (!this._map.getLayer(copy.layerId)) {
+      this._createLayer(copy, !sourceExisted);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/core/mapLayers.ts` around lines 154 - 156, When restoring a layer
(`restoreLayer`) we must not delete a pre-existing/native source if
`_createLayer` fails; update `_createLayer` (and call sites from `restoreLayer`)
so it knows whether it actually created `copy.sourceId` (e.g., determine upfront
if source existed or return/throw with a flag). Only remove the source in
`_createLayer`'s error handler when that flag indicates this manager created it
(and verify the source still exists and matches the expected id before removal).
Use the symbols `_createLayer`, `restoreLayer`, and `copy.sourceId` to locate
the logic and add the created-by-manager check or pass a boolean from
`restoreLayer` to avoid deleting host-recreated/native sources.

// The host recreated the native layer; reconcile paint and layout
// so manager state and the map agree.
this._map.setPaintProperty(copy.layerId, 'raster-opacity', copy.opacity);
this._map.setLayoutProperty(copy.layerId, 'visibility', copy.visible ? 'visible' : 'none');
}

this._layers.set(copy.id, copy);
return copy;
}

/**
* Creates the native MapLibre source for an added layer according to
* the current render mode.
*
* @param entry - The added layer to create a source for
*/
private _createSource(entry: AddedLayer): void {
if (this._options.renderMode === 'image') {
const view = this._computeView(entry);
this._map.addSource(entry.sourceId, {
Expand All @@ -129,8 +186,8 @@ export class MapLayerManager {
this._ensureViewHandler();
} else {
const tileTemplate = buildTileTemplate(
service,
sublayerId,
entry.service,
entry.sublayerId,
{ tileSize: this._options.tileSize, imageFormat: this._options.imageFormat },
this._options.servicesUrl
);
Expand All @@ -142,10 +199,18 @@ export class MapLayerManager {
};
// Bounds keep MapLibre from requesting tiles far outside the data
// extent, which the EnviroAtlas server answers with slow 504s.
if (bounds) source.bounds = bounds;
if (entry.bounds) source.bounds = entry.bounds;
this._map.addSource(entry.sourceId, source);
}
}

/**
* Creates the native MapLibre raster layer for an added layer,
* removing the source if layer creation fails.
*
* @param entry - The added layer to create a layer for
*/
private _createLayer(entry: AddedLayer): void {
// Insert below the configured layer when it exists on the map
const beforeId =
this._options.beforeId && this._map.getLayer(this._options.beforeId) ? this._options.beforeId : undefined;
Expand All @@ -156,12 +221,12 @@ export class MapLayerManager {
type: 'raster',
source: entry.sourceId,
paint: {
'raster-opacity': opacity,
'raster-opacity': entry.opacity,
// Image-mode sources swap the whole picture on view changes;
// fading would flash the old extent during the swap.
...(this._options.renderMode === 'image' ? { 'raster-fade-duration': 0 } : {}),
},
layout: { visibility: 'visible' },
layout: { visibility: entry.visible ? 'visible' : 'none' },
},
beforeId
);
Expand All @@ -172,9 +237,6 @@ export class MapLayerManager {
}
throw error;
}

this._layers.set(id, entry);
return entry;
}

/**
Expand Down
111 changes: 111 additions & 0 deletions tests/control.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -373,4 +373,115 @@ describe('EnviroAtlasControl', () => {
control.onRemove();
expect(mapContainer.querySelector('.enviroatlas-panel')).toBeNull();
});

const RESTORE_ENTRY = {
id: 'enviroatlas-restored',
sourceId: 'enviroatlas-restored',
layerId: 'enviroatlas-restored',
service: { folder: 'Supplemental', name: 'PADUS', fullName: 'Supplemental/PADUS', type: 'MapServer' as const },
sublayerId: 0,
label: 'PADUS 2.0',
visible: false,
opacity: 0.5,
bounds: [-130, 20, -60, 55] as [number, number, number, number],
};

it('reuses existing native source/layer when restoring and reconciles opacity/visibility', () => {
const { control } = mount({ collapsed: false });
const map = control.getMap()!;
// Host already recreated the natives before activating the control.
(map.getSource as ReturnType<typeof vi.fn>).mockImplementation((id: string) =>
id === RESTORE_ENTRY.sourceId ? { type: 'image' } : undefined
);
(map.getLayer as ReturnType<typeof vi.fn>).mockImplementation((id: string) =>
id === RESTORE_ENTRY.layerId ? { id } : undefined
);

control.restoreLayers([{ ...RESTORE_ENTRY }]);

// No duplicate natives created
expect(map.addSource).not.toHaveBeenCalled();
expect(map.addLayer).not.toHaveBeenCalled();
// Opacity and visibility applied to the existing native layer
expect(map.setPaintProperty).toHaveBeenCalledWith(RESTORE_ENTRY.layerId, 'raster-opacity', 0.5);
expect(map.setLayoutProperty).toHaveBeenCalledWith(RESTORE_ENTRY.layerId, 'visibility', 'none');

const layers = control.getState().addedLayers;
expect(layers).toHaveLength(1);
expect(layers[0].id).toBe(RESTORE_ENTRY.id);
expect(layers[0].opacity).toBe(0.5);
expect(layers[0].visible).toBe(false);
control.onRemove();
});

it('creates native source/layer when missing on restore (tiles mode)', () => {
const { control } = mount({ collapsed: false, renderMode: 'tiles' });
const map = control.getMap()!;
// Natives do not exist yet (default getSource/getLayer return undefined).

control.restoreLayers([{ ...RESTORE_ENTRY }]);

const sourceSpec = (map.addSource as ReturnType<typeof vi.fn>).mock.calls[0][1];
expect(sourceSpec.type).toBe('raster');
expect(sourceSpec.tiles[0]).toContain('{bbox-epsg-3857}');
expect(sourceSpec.bounds).toEqual(RESTORE_ENTRY.bounds);

const addLayerSpec = (map.addLayer as ReturnType<typeof vi.fn>).mock.calls[0][0];
expect(addLayerSpec.id).toBe(RESTORE_ENTRY.layerId);
expect(addLayerSpec.paint['raster-opacity']).toBe(0.5);
expect(addLayerSpec.layout.visibility).toBe('none');
control.onRemove();
});

it('restoreLayers skips duplicates and emits layeradd + statechange', () => {
const { control } = mount({ collapsed: false });
const map = control.getMap()!;
(map.getSource as ReturnType<typeof vi.fn>).mockReturnValue({ type: 'image' });
(map.getLayer as ReturnType<typeof vi.fn>).mockImplementation((id: string) =>
id === RESTORE_ENTRY.layerId ? { id } : undefined
);

const added: unknown[] = [];
let stateChanges = 0;
control.on('layeradd', (e) => added.push(e.layer));
control.on('statechange', () => stateChanges++);

// Same entry twice: the second is a duplicate by service+sublayer.
control.restoreLayers([{ ...RESTORE_ENTRY }, { ...RESTORE_ENTRY, id: 'enviroatlas-other' }]);

expect(added).toHaveLength(1);
// A single statechange for the whole batch
expect(stateChanges).toBe(1);
expect(control.getState().addedLayers).toHaveLength(1);
control.onRemove();
});

it('does not emit when restoreLayers restores nothing', () => {
const { control } = mount({ collapsed: false });
let stateChanges = 0;
control.on('statechange', () => stateChanges++);
control.restoreLayers([]);
expect(stateChanges).toBe(0);
control.onRemove();
});

it('defers restoreLayers called before onAdd and applies it after the control is added', () => {
const { map, controlCorner } = createFakeMap();
(map.getSource as ReturnType<typeof vi.fn>).mockReturnValue({ type: 'image' });
(map.getLayer as ReturnType<typeof vi.fn>).mockImplementation((id: string) =>
id === RESTORE_ENTRY.layerId ? { id } : undefined
);

const control = new EnviroAtlasControl({ collapsed: false });
// Called before the control is on a map: should defer, not throw.
control.restoreLayers([{ ...RESTORE_ENTRY }]);
expect(control.getState().addedLayers).toHaveLength(0);

const container = control.onAdd(map);
controlCorner.appendChild(container);

expect(control.getState().addedLayers).toHaveLength(1);
expect(control.getState().addedLayers[0].id).toBe(RESTORE_ENTRY.id);
control.onRemove();
});
});
Loading