feat: add restoreLayers API to rehydrate host-persisted layers#3
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds layer persistence restoration to EnviroAtlasControl. MapLayerManager now eagerly creates native sources and layers and provides a ChangesLayer Restoration Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/lib/core/EnviroAtlasControl.ts`:
- Around line 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.
- Around line 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.
In `@src/lib/core/mapLayers.ts`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3667ad70-a6a4-4a29-afc7-186025806de2
📒 Files selected for processing (5)
README.mdpackage.jsonsrc/lib/core/EnviroAtlasControl.tssrc/lib/core/mapLayers.tstests/control.test.ts
Comment on lines
+413
to
+418
| 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 }); | ||
| } |
There was a problem hiding this comment.
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.
Comment on lines
+413
to
+423
| 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 }); | ||
| } | ||
|
|
||
| if (!restored) return; | ||
| this._state.addedLayers = this._layerManager.getLayers(); | ||
| this._addedView?.update(this._state.addedLayers); | ||
| this._emit('statechange'); |
There was a problem hiding this comment.
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.
Comment on lines
+154
to
+156
| if (!this._map.getLayer(copy.layerId)) { | ||
| this._createLayer(copy); | ||
| } else { |
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Host applications that save and restore map state (for example a project file) persist the control's added layers and recreate the native MapLibre source/layer themselves on reload, before the control is re-activated. Until now there was no way to hand those layers back to the control:
MapLayerManager.addLayeralways generates fresh random ids, which would duplicate the host-recreated natives.API
MapLayerManager.restoreLayer(entry: AddedLayer): AddedLayeropacity(viasetPaintProperty) andvisible(viasetLayoutProperty) so manager state and the map agree.addLayerinto private_createSource/_createLayerhelpers used by both methods;addLayerbehavior is unchanged.EnviroAtlasControl.restoreLayers(entries: AddedLayer[]): voidrestoreLayerfor the rest and emitslayeraddper restored layer.state.addedLayers, refreshes the added-layers view, and emits a singlestatechangefor the batch.onAdd.Tests
Added vitest coverage for: reusing existing natives (no duplication) with opacity/visibility reconciliation, creating natives when missing (tiles mode), duplicate-skipping with a single batched
statechange, no-op emission, and deferred restore applied afteronAdd.Version bumped 0.1.0 -> 0.1.1 and README method table updated. Not published to npm.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores