mirror of
https://github.com/koala73/worldmonitor.git
synced 2026-04-25 17:14:57 +02:00
fix(map): stop deckgl layer toggles from getting stuck (#1248)
* fix(map): isolate deckgl layer state from external mutations * fix(map): improve state isolation and replace brittle regex test - Also shallow-copy pan in constructor for full isolation - Simplify setLayers by removing intermediate nextLayers variable - Replace regex source-scan test with negative-match assertions that survive refactors while still catching direct assignment * fix(map): complete state isolation — getState and onStateChange - getState() now deep-copies layers and pan, not just shallow spread - onStateChange callbacks pass this.getState() instead of this.state - Expanded test to cover getState copy and onStateChange isolation * test(map): replace source-scan test with behavioral isolation tests Source-text regex tests are brittle and don't prove runtime behavior. New tests replicate the exact copy logic from DeckGLMap's constructor, setLayers, getState, and onStateChange, then verify mutations to caller-owned objects never affect internal state and vice versa. --------- Co-authored-by: Elie Habib <elie.habib@gmail.com>
This commit is contained in:
@@ -406,7 +406,11 @@ export class DeckGLMap {
|
|||||||
|
|
||||||
constructor(container: HTMLElement, initialState: DeckMapState) {
|
constructor(container: HTMLElement, initialState: DeckMapState) {
|
||||||
this.container = container;
|
this.container = container;
|
||||||
this.state = initialState;
|
this.state = {
|
||||||
|
...initialState,
|
||||||
|
pan: { ...initialState.pan },
|
||||||
|
layers: { ...initialState.layers },
|
||||||
|
};
|
||||||
this.hotspots = [...INTEL_HOTSPOTS];
|
this.hotspots = [...INTEL_HOTSPOTS];
|
||||||
|
|
||||||
this.debouncedRebuildLayers = debounce(() => {
|
this.debouncedRebuildLayers = debounce(() => {
|
||||||
@@ -674,7 +678,7 @@ export class DeckGLMap {
|
|||||||
this.debouncedFetchBases();
|
this.debouncedFetchBases();
|
||||||
this.debouncedFetchAircraft();
|
this.debouncedFetchAircraft();
|
||||||
this.state.zoom = this.maplibreMap?.getZoom() ?? this.state.zoom;
|
this.state.zoom = this.maplibreMap?.getZoom() ?? this.state.zoom;
|
||||||
this.onStateChange?.(this.state);
|
this.onStateChange?.(this.getState());
|
||||||
});
|
});
|
||||||
|
|
||||||
this.maplibreMap.on('move', () => {
|
this.maplibreMap.on('move', () => {
|
||||||
@@ -701,7 +705,7 @@ export class DeckGLMap {
|
|||||||
this.debouncedRebuildLayers();
|
this.debouncedRebuildLayers();
|
||||||
}
|
}
|
||||||
this.state.zoom = this.maplibreMap?.getZoom() ?? this.state.zoom;
|
this.state.zoom = this.maplibreMap?.getZoom() ?? this.state.zoom;
|
||||||
this.onStateChange?.(this.state);
|
this.onStateChange?.(this.getState());
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -3968,7 +3972,7 @@ export class DeckGLMap {
|
|||||||
const viewSelect = this.container.querySelector('.view-select') as HTMLSelectElement;
|
const viewSelect = this.container.querySelector('.view-select') as HTMLSelectElement;
|
||||||
if (viewSelect) viewSelect.value = view;
|
if (viewSelect) viewSelect.value = view;
|
||||||
|
|
||||||
this.onStateChange?.(this.state);
|
this.onStateChange?.(this.getState());
|
||||||
}
|
}
|
||||||
|
|
||||||
public setZoom(zoom: number): void {
|
public setZoom(zoom: number): void {
|
||||||
@@ -4020,19 +4024,22 @@ export class DeckGLMap {
|
|||||||
}
|
}
|
||||||
|
|
||||||
public setLayers(layers: MapLayers): void {
|
public setLayers(layers: MapLayers): void {
|
||||||
this.state.layers = layers;
|
this.state.layers = { ...layers };
|
||||||
this.manageAircraftTimer(layers.flights);
|
this.manageAircraftTimer(this.state.layers.flights);
|
||||||
this.render(); // Debounced
|
this.render(); // Debounced
|
||||||
|
|
||||||
// Update toggle checkboxes
|
Object.entries(this.state.layers).forEach(([key, value]) => {
|
||||||
Object.entries(layers).forEach(([key, value]) => {
|
|
||||||
const toggle = this.container.querySelector(`.layer-toggle[data-layer="${key}"] input`) as HTMLInputElement;
|
const toggle = this.container.querySelector(`.layer-toggle[data-layer="${key}"] input`) as HTMLInputElement;
|
||||||
if (toggle) toggle.checked = value;
|
if (toggle) toggle.checked = value;
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
public getState(): DeckMapState {
|
public getState(): DeckMapState {
|
||||||
return { ...this.state };
|
return {
|
||||||
|
...this.state,
|
||||||
|
pan: { ...this.state.pan },
|
||||||
|
layers: { ...this.state.layers },
|
||||||
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
// Zoom controls - public for external access
|
// Zoom controls - public for external access
|
||||||
|
|||||||
138
tests/deckgl-layer-state-aliasing.test.mjs
Normal file
138
tests/deckgl-layer-state-aliasing.test.mjs
Normal file
@@ -0,0 +1,138 @@
|
|||||||
|
/**
|
||||||
|
* Behavioral tests for DeckGLMap state isolation.
|
||||||
|
*
|
||||||
|
* DeckGLMap requires DOM + WebGL so it cannot be instantiated in Node.
|
||||||
|
* These tests replicate the exact copy logic used in the constructor,
|
||||||
|
* setLayers(), getState(), and onStateChange() to prove the isolation
|
||||||
|
* contract holds at runtime — any mutation to caller-owned objects must
|
||||||
|
* NOT affect internal state, and vice versa.
|
||||||
|
*/
|
||||||
|
import { describe, it } from 'node:test';
|
||||||
|
import assert from 'node:assert/strict';
|
||||||
|
|
||||||
|
// ---------- helpers replicating DeckGLMap logic ----------
|
||||||
|
|
||||||
|
function copyInitialState(initialState) {
|
||||||
|
return {
|
||||||
|
...initialState,
|
||||||
|
pan: { ...initialState.pan },
|
||||||
|
layers: { ...initialState.layers },
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
function copyLayers(layers) {
|
||||||
|
return { ...layers };
|
||||||
|
}
|
||||||
|
|
||||||
|
function copyStateForExport(state) {
|
||||||
|
return {
|
||||||
|
...state,
|
||||||
|
pan: { ...state.pan },
|
||||||
|
layers: { ...state.layers },
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
// ---------- fixtures ----------
|
||||||
|
|
||||||
|
function makeState() {
|
||||||
|
return {
|
||||||
|
zoom: 3,
|
||||||
|
pan: { x: 10, y: 20 },
|
||||||
|
view: 'global',
|
||||||
|
layers: { hotspots: true, flights: false, conflicts: true },
|
||||||
|
timeRange: '24h',
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
// ---------- tests ----------
|
||||||
|
|
||||||
|
describe('DeckGLMap state isolation (behavioral)', () => {
|
||||||
|
describe('constructor isolation', () => {
|
||||||
|
it('mutating the original layers object does not affect internal state', () => {
|
||||||
|
const original = makeState();
|
||||||
|
const internal = copyInitialState(original);
|
||||||
|
original.layers.hotspots = false;
|
||||||
|
assert.equal(internal.layers.hotspots, true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('mutating the original pan object does not affect internal state', () => {
|
||||||
|
const original = makeState();
|
||||||
|
const internal = copyInitialState(original);
|
||||||
|
original.pan.x = 999;
|
||||||
|
assert.equal(internal.pan.x, 10);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('mutating internal state does not affect the original', () => {
|
||||||
|
const original = makeState();
|
||||||
|
const internal = copyInitialState(original);
|
||||||
|
internal.layers.flights = true;
|
||||||
|
assert.equal(original.layers.flights, false);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('setLayers isolation', () => {
|
||||||
|
it('mutating the input layers after setLayers does not affect stored layers', () => {
|
||||||
|
const input = { hotspots: true, flights: false, conflicts: true };
|
||||||
|
const stored = copyLayers(input);
|
||||||
|
input.hotspots = false;
|
||||||
|
assert.equal(stored.hotspots, true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('mutating stored layers does not affect the caller object', () => {
|
||||||
|
const input = { hotspots: true, flights: false, conflicts: true };
|
||||||
|
const stored = copyLayers(input);
|
||||||
|
stored.flights = true;
|
||||||
|
assert.equal(input.flights, false);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('getState isolation', () => {
|
||||||
|
it('returned state.layers is a separate object from internal layers', () => {
|
||||||
|
const internal = { state: makeState() };
|
||||||
|
const exported = copyStateForExport(internal.state);
|
||||||
|
assert.notEqual(exported.layers, internal.state.layers);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('mutating returned layers does not affect internal state', () => {
|
||||||
|
const internal = { state: makeState() };
|
||||||
|
const exported = copyStateForExport(internal.state);
|
||||||
|
exported.layers.hotspots = false;
|
||||||
|
assert.equal(internal.state.layers.hotspots, true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returned state.pan is a separate object from internal pan', () => {
|
||||||
|
const internal = { state: makeState() };
|
||||||
|
const exported = copyStateForExport(internal.state);
|
||||||
|
assert.notEqual(exported.pan, internal.state.pan);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('mutating returned pan does not affect internal state', () => {
|
||||||
|
const internal = { state: makeState() };
|
||||||
|
const exported = copyStateForExport(internal.state);
|
||||||
|
exported.pan.x = 999;
|
||||||
|
assert.equal(internal.state.pan.x, 10);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('onStateChange isolation', () => {
|
||||||
|
it('callback receives a copy, not the internal reference', () => {
|
||||||
|
const internal = { state: makeState() };
|
||||||
|
let received = null;
|
||||||
|
const callback = (s) => { received = s; };
|
||||||
|
callback(copyStateForExport(internal.state));
|
||||||
|
assert.notEqual(received.layers, internal.state.layers);
|
||||||
|
assert.notEqual(received.pan, internal.state.pan);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('mutating the callback state does not affect internal state', () => {
|
||||||
|
const internal = { state: makeState() };
|
||||||
|
let received = null;
|
||||||
|
const callback = (s) => { received = s; };
|
||||||
|
callback(copyStateForExport(internal.state));
|
||||||
|
received.layers.hotspots = false;
|
||||||
|
received.pan.x = 999;
|
||||||
|
assert.equal(internal.state.layers.hotspots, true);
|
||||||
|
assert.equal(internal.state.pan.x, 10);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user