From e53a43e494f755aa44d2d5536ead630dc5763a1d Mon Sep 17 00:00:00 2001 From: Frank <97429702+tsubasakong@users.noreply.github.com> Date: Sat, 7 Mar 2026 21:33:49 -0800 Subject: [PATCH] fix(map): stop deckgl layer toggles from getting stuck (#1248) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- src/components/DeckGLMap.ts | 25 ++-- tests/deckgl-layer-state-aliasing.test.mjs | 138 +++++++++++++++++++++ 2 files changed, 154 insertions(+), 9 deletions(-) create mode 100644 tests/deckgl-layer-state-aliasing.test.mjs diff --git a/src/components/DeckGLMap.ts b/src/components/DeckGLMap.ts index e575ae9d0..7199399a7 100644 --- a/src/components/DeckGLMap.ts +++ b/src/components/DeckGLMap.ts @@ -406,7 +406,11 @@ export class DeckGLMap { constructor(container: HTMLElement, initialState: DeckMapState) { this.container = container; - this.state = initialState; + this.state = { + ...initialState, + pan: { ...initialState.pan }, + layers: { ...initialState.layers }, + }; this.hotspots = [...INTEL_HOTSPOTS]; this.debouncedRebuildLayers = debounce(() => { @@ -674,7 +678,7 @@ export class DeckGLMap { this.debouncedFetchBases(); this.debouncedFetchAircraft(); this.state.zoom = this.maplibreMap?.getZoom() ?? this.state.zoom; - this.onStateChange?.(this.state); + this.onStateChange?.(this.getState()); }); this.maplibreMap.on('move', () => { @@ -701,7 +705,7 @@ export class DeckGLMap { this.debouncedRebuildLayers(); } 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; if (viewSelect) viewSelect.value = view; - this.onStateChange?.(this.state); + this.onStateChange?.(this.getState()); } public setZoom(zoom: number): void { @@ -4020,19 +4024,22 @@ export class DeckGLMap { } public setLayers(layers: MapLayers): void { - this.state.layers = layers; - this.manageAircraftTimer(layers.flights); + this.state.layers = { ...layers }; + this.manageAircraftTimer(this.state.layers.flights); this.render(); // Debounced - // Update toggle checkboxes - Object.entries(layers).forEach(([key, value]) => { + Object.entries(this.state.layers).forEach(([key, value]) => { const toggle = this.container.querySelector(`.layer-toggle[data-layer="${key}"] input`) as HTMLInputElement; if (toggle) toggle.checked = value; }); } public getState(): DeckMapState { - return { ...this.state }; + return { + ...this.state, + pan: { ...this.state.pan }, + layers: { ...this.state.layers }, + }; } // Zoom controls - public for external access diff --git a/tests/deckgl-layer-state-aliasing.test.mjs b/tests/deckgl-layer-state-aliasing.test.mjs new file mode 100644 index 000000000..2b1ed27b6 --- /dev/null +++ b/tests/deckgl-layer-state-aliasing.test.mjs @@ -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); + }); + }); +});