mirror of
https://github.com/koala73/worldmonitor.git
synced 2026-04-25 17:14:57 +02:00
fix(url-sync): don't overwrite URL position params on initial load (#2580)
* fix(url-sync): don't overwrite URL position params on initial load Race condition: setupUrlStateSync() called debouncedUrlSync() immediately, but applyInitialUrlState() had already started an async flyTo via setCenter(). At ~250ms the debounce fired while the flyTo was still running, reading the map's default center (lat=20, lon=0, zoom=1) and writing it back over the URL-specified params. Fix: skip the immediate debouncedUrlSync() call when URL position params are present. onStateChanged fires on moveend (after flyTo completes) and handles the first URL write at that point with the correct position. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(url-sync): pass URL zoom to setView and always apply URL lat/lon When the URL contains ?view=global&zoom=1.00, setView was called without a zoom override, so flyTo animated to the preset zoom (1.5) instead. The subsequent moveend/onStateChange cycle then wrote zoom=1.50 back to the URL, overwriting the correct zoom=1.00. Pass the URL zoom as the second argument to setView so flyTo targets the correct zoom level. Also remove the effectiveZoom > 2 guard from the lat/lon branch so URL lat/lon is always honoured regardless of zoom level. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(url-sync): implement setView(view, zoom?) across all map implementations DeckGLMap, GlobeMap, Map, and MapContainer setView() only accepted a single view argument — the zoom passed from panel-layout was silently ignored, so ?view=X&zoom=Y still wrote the preset zoom back to the URL after moveend. - DeckGLMap: pass zoom ?? preset.zoom into flyTo - GlobeMap: apply same deck.gl→altitude mapping as setCenter when zoom provided - Map (SVG): use zoom ?? settings.zoom for the SVG map zoom state - MapContainer: forward the optional zoom to all underlying implementations * fix(url-sync): forward zoom on SVG path + tighten urlHasAsyncFlyTo guard P1: MapContainer was forwarding zoom to DeckGL/Globe but not to the SVG fallback renderer — ?view=X&zoom=Y was still loading at preset zoom on mobile/WebGL-fallback. P2: urlHasPosition fired on any single URL param including lone ?lat or ?lon. applyInitialUrlState only calls setCenter when both are present, so malformed links like ?lat=41 skipped canonicalization and left stale params in the URL until the user moved the map. Fix: rename to urlHasAsyncFlyTo and only suppress the initial debounced sync when applyInitialUrlState will actually trigger an async operation: - view is set (setView → flyTo) - both lat AND lon are set (setCenter → flyTo) - zoom is set (setZoom → animated zoom) * fix(url-sync): remove view from urlHasAsyncFlyTo suppression guard view was included in the suppression condition but this breaks two renderer paths: - SVG/Map: setView() is synchronous and fires emitStateChange before the URL-sync listener is installed (App.ts wires listeners after applyInitialUrlState runs). No async callback ever fires to drive the URL write when suppressed. - GlobeMap: onStateChanged() is a no-op stub — suppressing the initial debounce leaves /?view=X deep links without any URL sync on the globe renderer. view state is written synchronously at the top of every setView() implementation (this.state.view = view), so the 250ms debounced read is always correct regardless of renderer. Remove view from the guard. Only suppress for the two cases where an in-flight flyTo makes getCenter() return stale intermediate coordinates: (a) lat+lon pair present → setCenter() flyTo (b) zoom-only without a view preset → setZoom() animated zoom * fix(url-sync): eagerly cache target center+zoom in DeckGLMap.setView DeckGLMap.setView() started a flyTo without updating this.state.zoom or this.pendingCenter. The 250ms URL debounce would then read: - state.zoom → old cached value (wrong zoom written to URL) - getCenter() → maplibreMap.getCenter() mid-animation (intermediate coords written to URL, e.g. lat=22.1 instead of 25.0) Fix: at the top of setView(), before flyTo starts: - Set this.state.zoom = zoom ?? preset.zoom (zoom correct immediately) - Set this.pendingCenter = preset lat/lon (center correct immediately) - getCenter() returns pendingCenter when set, falls through to live maplibre coords after moveend clears it This ensures any URL sync that fires during the flyTo animation (whether the initial 250ms debounce or an onStateChanged triggered by the sync setView call itself) reads the correct destination values. * test(url-sync): regression coverage for initial URL-sync suppression + DeckGLMap.pendingCenter 20 tests across three suites: - urlHasAsyncFlyTo guard: 10 cases covering cold load, view-only, partial lat/lon, full lat+lon, bare zoom, view+zoom, and the setCenter override path. - DeckGLMap.pendingCenter: 8 cases verifying eager center/zoom cache on setView, override zoom, consecutive calls, and moveend clearing the cache. - Integration regression: view=mena path confirms suppression is skipped and pendingCenter holds correct preset coords for the URL builder. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Elie Habib <elie.habib@gmail.com>
This commit is contained in:
@@ -700,7 +700,27 @@ export class EventHandlerManager implements AppModule {
|
||||
}
|
||||
this.debouncedWebcamReload();
|
||||
});
|
||||
this.debouncedUrlSync();
|
||||
|
||||
// Skip the immediate sync only when applyInitialUrlState() will start an
|
||||
// async flyTo that makes getCenter() return stale intermediate coordinates.
|
||||
// Two cases qualify:
|
||||
// (a) lat+lon pair → setCenter() flyTo; both must be present since
|
||||
// applyInitialUrlState only calls setCenter when both exist.
|
||||
// (b) bare zoom → setZoom() animated zoom (no view preset).
|
||||
//
|
||||
// view is intentionally excluded: all renderers set this.state.view
|
||||
// synchronously at the top of setView(), so the debounced read is always
|
||||
// correct regardless of renderer. GlobeMap.onStateChanged is a no-op and
|
||||
// SVG Map fires emitStateChange before the listener is installed — neither
|
||||
// can rely on a later onStateChanged to drive the URL write, so they must
|
||||
// use the immediate debounce path.
|
||||
const { view, lat, lon, zoom } = this.ctx.initialUrlState ?? {};
|
||||
const urlHasAsyncFlyTo =
|
||||
(lat !== undefined && lon !== undefined) || // setCenter → flyTo (requires both)
|
||||
(!view && zoom !== undefined); // zoom-only → setZoom animated
|
||||
if (!urlHasAsyncFlyTo) {
|
||||
this.debouncedUrlSync();
|
||||
}
|
||||
}
|
||||
|
||||
syncUrlState(): void {
|
||||
|
||||
@@ -1335,7 +1335,8 @@ export class PanelLayoutManager implements AppModule {
|
||||
const { view, zoom, lat, lon, timeRange, layers } = this.ctx.initialUrlState;
|
||||
|
||||
if (view) {
|
||||
this.ctx.map.setView(view);
|
||||
// Pass URL zoom so the preset's default zoom doesn't overwrite it.
|
||||
this.ctx.map.setView(view, zoom);
|
||||
}
|
||||
|
||||
if (timeRange) {
|
||||
@@ -1349,9 +1350,10 @@ export class PanelLayoutManager implements AppModule {
|
||||
}
|
||||
|
||||
if (lat !== undefined && lon !== undefined) {
|
||||
const effectiveZoom = zoom ?? this.ctx.map.getState().zoom;
|
||||
if (effectiveZoom > 2) this.ctx.map.setCenter(lat, lon, zoom);
|
||||
// Always honour URL lat/lon regardless of zoom level.
|
||||
this.ctx.map.setCenter(lat, lon, zoom);
|
||||
} else if (!view && zoom !== undefined) {
|
||||
// zoom-only without a view preset: apply directly.
|
||||
this.ctx.map.setZoom(zoom);
|
||||
}
|
||||
|
||||
|
||||
@@ -483,6 +483,10 @@ export class DeckGLMap {
|
||||
private handleThemeChange: () => void;
|
||||
private handleMapThemeChange: () => void;
|
||||
private moveTimeoutId: ReturnType<typeof setTimeout> | null = null;
|
||||
/** Target center set eagerly by setView() so getCenter() returns the correct
|
||||
* destination before moveend fires, preventing stale intermediate coords
|
||||
* from being written to the URL during flyTo. Cleared on moveend. */
|
||||
private pendingCenter: { lat: number; lon: number } | null = null;
|
||||
private lastAircraftFetchCenter: [number, number] | null = null;
|
||||
private lastAircraftFetchZoom = -1;
|
||||
private aircraftFetchSeq = 0;
|
||||
@@ -833,6 +837,7 @@ export class DeckGLMap {
|
||||
});
|
||||
|
||||
this.maplibreMap.on('moveend', () => {
|
||||
this.pendingCenter = null;
|
||||
this.lastSCZoom = -1;
|
||||
this.rafUpdateLayers();
|
||||
this.debouncedFetchBases();
|
||||
@@ -4619,15 +4624,21 @@ export class DeckGLMap {
|
||||
}
|
||||
}
|
||||
|
||||
public setView(view: DeckMapView): void {
|
||||
public setView(view: DeckMapView, zoom?: number): void {
|
||||
const preset = VIEW_PRESETS[view];
|
||||
if (!preset) return;
|
||||
this.state.view = view;
|
||||
// Eagerly write target zoom+center so getState()/getCenter() return the
|
||||
// correct destination before moveend fires. Without this a 250ms URL sync
|
||||
// reads the old cached zoom or an intermediate animated center and
|
||||
// overwrites URL params (e.g. ?view=mena&zoom=4 → wrong coords).
|
||||
this.state.zoom = zoom ?? preset.zoom;
|
||||
this.pendingCenter = { lat: preset.latitude, lon: preset.longitude };
|
||||
|
||||
if (this.maplibreMap) {
|
||||
this.maplibreMap.flyTo({
|
||||
center: [preset.longitude, preset.latitude],
|
||||
zoom: preset.zoom,
|
||||
zoom: this.state.zoom,
|
||||
duration: 1000,
|
||||
});
|
||||
}
|
||||
@@ -4667,6 +4678,7 @@ export class DeckGLMap {
|
||||
}
|
||||
|
||||
public getCenter(): { lat: number; lon: number } | null {
|
||||
if (this.pendingCenter) return this.pendingCenter;
|
||||
if (this.maplibreMap) {
|
||||
const center = this.maplibreMap.getCenter();
|
||||
return { lat: center.lat, lon: center.lng };
|
||||
|
||||
@@ -2551,12 +2551,21 @@ export class GlobeMap {
|
||||
oceania: { lat: -25, lng: 140, altitude: 1.5 },
|
||||
};
|
||||
|
||||
public setView(view: MapView): void {
|
||||
public setView(view: MapView, zoom?: number): void {
|
||||
this.currentView = view;
|
||||
if (!this.globe) return;
|
||||
this.wakeGlobe();
|
||||
const pov = GlobeMap.VIEW_POVS[view] ?? GlobeMap.VIEW_POVS.global;
|
||||
this.globe.pointOfView(pov, 1200);
|
||||
const preset = GlobeMap.VIEW_POVS[view] ?? GlobeMap.VIEW_POVS.global;
|
||||
let altitude = preset.altitude;
|
||||
if (zoom !== undefined) {
|
||||
if (zoom >= 7) altitude = 0.08;
|
||||
else if (zoom >= 6) altitude = 0.15;
|
||||
else if (zoom >= 5) altitude = 0.3;
|
||||
else if (zoom >= 4) altitude = 0.5;
|
||||
else if (zoom >= 3) altitude = 0.8;
|
||||
else altitude = 1.5;
|
||||
}
|
||||
this.globe.pointOfView({ lat: preset.lat, lng: preset.lng, altitude }, 1200);
|
||||
}
|
||||
|
||||
public setCenter(lat: number, lon: number, zoom?: number): void {
|
||||
|
||||
@@ -3331,7 +3331,7 @@ export class MapComponent {
|
||||
return getHotspotEscalation(hotspotId);
|
||||
}
|
||||
|
||||
public setView(view: MapView): void {
|
||||
public setView(view: MapView, zoom?: number): void {
|
||||
this.state.view = view;
|
||||
|
||||
// Region-specific zoom and pan settings
|
||||
@@ -3348,7 +3348,7 @@ export class MapComponent {
|
||||
};
|
||||
|
||||
const settings = viewSettings[view];
|
||||
this.state.zoom = settings.zoom;
|
||||
this.state.zoom = zoom ?? settings.zoom;
|
||||
this.state.pan = settings.pan;
|
||||
this.applyTransform();
|
||||
this.render();
|
||||
|
||||
@@ -349,9 +349,9 @@ export class MapContainer {
|
||||
if (this.useDeckGL) { this.deckGLMap?.setIsResizing(isResizing); } else { this.svgMap?.setIsResizing(isResizing); }
|
||||
}
|
||||
|
||||
public setView(view: MapView): void {
|
||||
if (this.useGlobe) { this.globeMap?.setView(view); return; }
|
||||
if (this.useDeckGL) { this.deckGLMap?.setView(view as DeckMapView); } else { this.svgMap?.setView(view); }
|
||||
public setView(view: MapView, zoom?: number): void {
|
||||
if (this.useGlobe) { this.globeMap?.setView(view, zoom); return; }
|
||||
if (this.useDeckGL) { this.deckGLMap?.setView(view as DeckMapView, zoom); } else { this.svgMap?.setView(view, zoom); }
|
||||
}
|
||||
|
||||
public setZoom(zoom: number): void {
|
||||
|
||||
211
tests/url-sync-initial.test.mts
Normal file
211
tests/url-sync-initial.test.mts
Normal file
@@ -0,0 +1,211 @@
|
||||
/**
|
||||
* Regression tests for initial URL-sync suppression and DeckGLMap.pendingCenter.
|
||||
*
|
||||
* These cover the bugs fixed in the fix/url-params-overwrite series:
|
||||
* - urlHasAsyncFlyTo guard (event-handlers.ts setupUrlStateSync)
|
||||
* - DeckGLMap.pendingCenter eager cache (prevents stale center during flyTo)
|
||||
*/
|
||||
|
||||
import { describe, it } from 'node:test';
|
||||
import assert from 'node:assert/strict';
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Inline the pure urlHasAsyncFlyTo logic so these tests are zero-dependency
|
||||
// (no DOM, no maplibre). This mirrors the exact condition in setupUrlStateSync.
|
||||
// ---------------------------------------------------------------------------
|
||||
function urlHasAsyncFlyTo(
|
||||
initialUrlState: { view?: string; lat?: number; lon?: number; zoom?: number } | undefined,
|
||||
): boolean {
|
||||
const { view, lat, lon, zoom } = initialUrlState ?? {};
|
||||
return (
|
||||
(lat !== undefined && lon !== undefined) || // setCenter → flyTo (both required)
|
||||
(!view && zoom !== undefined) // zoom-only → setZoom animated
|
||||
);
|
||||
}
|
||||
|
||||
describe('urlHasAsyncFlyTo — suppression guard', () => {
|
||||
it('returns false when initialUrlState is undefined (cold load)', () => {
|
||||
assert.equal(urlHasAsyncFlyTo(undefined), false);
|
||||
});
|
||||
|
||||
it('returns false for bare ?view=mena (no lat/lon, no zoom)', () => {
|
||||
assert.equal(urlHasAsyncFlyTo({ view: 'mena' }), false);
|
||||
});
|
||||
|
||||
it('returns false for lone ?lat=41 without lon', () => {
|
||||
// Partial params must NOT suppress the immediate sync — only a full lat+lon
|
||||
// pair triggers an async flyTo via setCenter().
|
||||
assert.equal(urlHasAsyncFlyTo({ lat: 41 }), false);
|
||||
});
|
||||
|
||||
it('returns false for lone ?lon=29 without lat', () => {
|
||||
assert.equal(urlHasAsyncFlyTo({ lon: 29 }), false);
|
||||
});
|
||||
|
||||
it('returns true for full ?lat=41&lon=29 pair', () => {
|
||||
// setCenter() is only called when both coords are present → async flyTo.
|
||||
assert.equal(urlHasAsyncFlyTo({ lat: 41, lon: 29 }), true);
|
||||
});
|
||||
|
||||
it('returns true for full lat+lon+zoom triplet', () => {
|
||||
assert.equal(urlHasAsyncFlyTo({ lat: 41, lon: 29, zoom: 6 }), true);
|
||||
});
|
||||
|
||||
it('returns true for bare ?zoom without view (animated setZoom)', () => {
|
||||
// No view preset means setZoom() is called, which animates the transition.
|
||||
assert.equal(urlHasAsyncFlyTo({ zoom: 5 }), true);
|
||||
});
|
||||
|
||||
it('returns false for ?view=mena&zoom=4 (view+zoom uses setView, synchronous)', () => {
|
||||
// When a view is present, setView() is used (not bare setZoom), so DeckGLMap
|
||||
// writes state.zoom eagerly — no suppression needed.
|
||||
assert.equal(urlHasAsyncFlyTo({ view: 'mena', zoom: 4 }), false);
|
||||
});
|
||||
|
||||
it('returns false for ?view=eu with lat+lon absent', () => {
|
||||
assert.equal(urlHasAsyncFlyTo({ view: 'eu' }), false);
|
||||
});
|
||||
|
||||
it('returns true for ?view=eu&lat=50&lon=15 (setCenter overrides view)', () => {
|
||||
// When lat+lon are present applyInitialUrlState calls setCenter regardless
|
||||
// of view — async flyTo path.
|
||||
assert.equal(urlHasAsyncFlyTo({ view: 'eu', lat: 50, lon: 15 }), true);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// DeckGLMap.pendingCenter behaviour — tested via a minimal in-process stub
|
||||
// that replicates the exact field logic without requiring maplibre or a DOM.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/** Minimal stub that mirrors only the pendingCenter + getCenter + setView logic. */
|
||||
class DeckGLMapStub {
|
||||
public state = { view: 'global', zoom: 1.5 };
|
||||
private pendingCenter: { lat: number; lon: number } | null = null;
|
||||
|
||||
private readonly VIEW_PRESETS: Record<string, { longitude: number; latitude: number; zoom: number }> = {
|
||||
global: { longitude: 0, latitude: 20, zoom: 1.5 },
|
||||
mena: { longitude: 45, latitude: 28, zoom: 3.5 },
|
||||
eu: { longitude: 15, latitude: 50, zoom: 3.5 },
|
||||
america:{ longitude: -95, latitude: 38, zoom: 3 },
|
||||
};
|
||||
|
||||
setView(view: string, zoom?: number): void {
|
||||
const preset = this.VIEW_PRESETS[view];
|
||||
if (!preset) return;
|
||||
this.state.view = view;
|
||||
this.state.zoom = zoom ?? preset.zoom;
|
||||
this.pendingCenter = { lat: preset.latitude, lon: preset.longitude };
|
||||
// (maplibreMap.flyTo would be called here in the real impl)
|
||||
}
|
||||
|
||||
/** Called by the real moveend listener. */
|
||||
simulateMoveEnd(finalLat: number, finalLon: number, finalZoom: number): void {
|
||||
this.pendingCenter = null;
|
||||
this.state.zoom = finalZoom;
|
||||
// (onStateChange?.(this.getState()) would fire here)
|
||||
}
|
||||
|
||||
getCenter(): { lat: number; lon: number } | null {
|
||||
if (this.pendingCenter) return this.pendingCenter;
|
||||
return null; // maplibreMap absent in stub
|
||||
}
|
||||
|
||||
getState() {
|
||||
return { view: this.state.view, zoom: this.state.zoom };
|
||||
}
|
||||
}
|
||||
|
||||
describe('DeckGLMap.pendingCenter — eager center cache', () => {
|
||||
it('setView sets pendingCenter to preset coords', () => {
|
||||
const m = new DeckGLMapStub();
|
||||
m.setView('mena');
|
||||
const c = m.getCenter();
|
||||
assert.ok(c, 'getCenter() must return non-null after setView');
|
||||
assert.equal(c.lat, 28);
|
||||
assert.equal(c.lon, 45);
|
||||
});
|
||||
|
||||
it('setView eagerly updates state.zoom to preset default', () => {
|
||||
const m = new DeckGLMapStub();
|
||||
m.setView('mena');
|
||||
assert.equal(m.getState().zoom, 3.5);
|
||||
});
|
||||
|
||||
it('setView with explicit zoom overrides preset zoom', () => {
|
||||
const m = new DeckGLMapStub();
|
||||
m.setView('mena', 4);
|
||||
assert.equal(m.getState().zoom, 4);
|
||||
// center must still be the preset's lat/lon
|
||||
const c = m.getCenter();
|
||||
assert.ok(c);
|
||||
assert.equal(c.lat, 28);
|
||||
assert.equal(c.lon, 45);
|
||||
});
|
||||
|
||||
it('getCenter returns pendingCenter before moveend fires', () => {
|
||||
const m = new DeckGLMapStub();
|
||||
m.setView('eu');
|
||||
const c = m.getCenter();
|
||||
assert.ok(c, 'must return pending center during flyTo animation');
|
||||
assert.equal(c.lat, 50);
|
||||
assert.equal(c.lon, 15);
|
||||
});
|
||||
|
||||
it('moveend clears pendingCenter', () => {
|
||||
const m = new DeckGLMapStub();
|
||||
m.setView('mena');
|
||||
m.simulateMoveEnd(28, 45, 3.5);
|
||||
// After moveend, pendingCenter is null — getCenter() falls through to
|
||||
// maplibreMap (absent in stub → null). Real impl would use maplibreMap.getCenter().
|
||||
assert.equal(m.getCenter(), null);
|
||||
});
|
||||
|
||||
it('moveend updates state.zoom to actual final zoom', () => {
|
||||
const m = new DeckGLMapStub();
|
||||
m.setView('mena', 4);
|
||||
// flyTo might settle at a slightly different zoom
|
||||
m.simulateMoveEnd(28, 45, 4.02);
|
||||
assert.equal(m.getState().zoom, 4.02);
|
||||
});
|
||||
|
||||
it('consecutive setView calls reset pendingCenter to new preset', () => {
|
||||
const m = new DeckGLMapStub();
|
||||
m.setView('mena');
|
||||
m.setView('eu');
|
||||
const c = m.getCenter();
|
||||
assert.ok(c);
|
||||
assert.equal(c.lat, 50);
|
||||
assert.equal(c.lon, 15);
|
||||
});
|
||||
|
||||
it('setView updates state.view synchronously', () => {
|
||||
const m = new DeckGLMapStub();
|
||||
m.setView('america');
|
||||
assert.equal(m.getState().view, 'america');
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Integration: urlHasAsyncFlyTo + pendingCenter interaction
|
||||
// Regression for: "?view=mena URL gained wrong lat/lon after initial sync"
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('regression: ?view=mena initial sync writes correct coords', () => {
|
||||
it('view-only URL does NOT suppress sync (urlHasAsyncFlyTo=false)', () => {
|
||||
// The listener must fire the immediate debounce so the URL is updated.
|
||||
assert.equal(urlHasAsyncFlyTo({ view: 'mena' }), false);
|
||||
});
|
||||
|
||||
it('pendingCenter holds preset coords during flyTo so buildMapUrl gets correct lat/lon', () => {
|
||||
const m = new DeckGLMapStub();
|
||||
// applyInitialUrlState calls setView('mena') → pendingCenter is set
|
||||
m.setView('mena');
|
||||
// When debouncedUrlSync fires (250ms) it calls map.getCenter()
|
||||
const center = m.getCenter();
|
||||
assert.ok(center, 'center must be available for URL builder');
|
||||
assert.equal(center.lat, 28, 'lat must be mena preset, not 0/20 global default');
|
||||
assert.equal(center.lon, 45, 'lon must be mena preset');
|
||||
assert.equal(m.getState().zoom, 3.5, 'zoom must be mena preset');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user