Skip to content

fix(url-sync): don't overwrite URL position params on initial load#2580

Merged
koala73 merged 8 commits intokoala73:mainfrom
rayanhabib07:fix/url-params-overwrite
Apr 1, 2026
Merged

fix(url-sync): don't overwrite URL position params on initial load#2580
koala73 merged 8 commits intokoala73:mainfrom
rayanhabib07:fix/url-params-overwrite

Conversation

@rayanhabib07
Copy link
Copy Markdown
Contributor

@rayanhabib07 rayanhabib07 commented Mar 31, 2026

Root cause

Race condition in setupUrlStateSync(). applyInitialUrlState() calls
map.setCenter() which starts an async flyTo animation. setupUrlStateSync()
was then immediately calling debouncedUrlSync() — 250ms later the debounce
fired while flyTo was still running, reading the default center
(lat=20, lon=0, zoom=1) and writing it back over the URL params.

Fix

Skip the immediate debouncedUrlSync() call when URL position params
(lat/lon/zoom) are present. onStateChanged fires on moveend after flyTo
completes, at which point getCenter() returns the correct position and
handles the first URL write.

Test plan

  • Load /?lat=41.0089&lon=0.0000&zoom=4 — URL params should persist after 1–2s
  • Load with no params — URL sync still works normally on map interactions
  • Load with only ?view=mena — immediate sync still fires (no position params)

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>
@github-actions github-actions Bot added the trust:caution Brin: contributor trust score caution label Mar 31, 2026
@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 31, 2026

@rayanhabib07 is attempting to deploy a commit to the Elie Team on Vercel.

A member of the Team first needs to authorize it.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 31, 2026

Greptile Summary

This PR fixes a race condition in setupUrlStateSync() where the initial debouncedUrlSync() call (250 ms debounce) would fire while a flyTo animation was still in progress, reading the default map center (lat=20, lon=0, zoom=1) and overwriting the URL params the user intended to share.

Key changes:

  • setupUrlStateSync() now checks whether lat, lon, or zoom are present in ctx.initialUrlState before firing the immediate debouncedUrlSync() call.
  • When position params are present, the immediate sync is skipped and the first correct URL write is delegated to the existing onStateChanged handler, which fires on moveend after the flyTo animation completes.
  • When no position params are present (e.g. ?view=mena), behavior is unchanged — debouncedUrlSync() fires immediately as before.
  • The logic correctly uses optional chaining (?.lat) to handle the null typed initialUrlState, and the undefined check aligns with how parseMapUrlState represents absent parameters.

Minor edge case (P2): When lat/lon are in the URL but effectiveZoom ≤ 2, the pre-existing guard in applyInitialUrlState() skips setCenter() — no animation fires, so onStateChanged never triggers the first URL write. The URL is preserved but stays out of sync with the actual map state until the next user interaction. This is a narrow edge case and unlikely to affect real users.

Confidence Score: 5/5

Safe to merge — the fix is narrowly scoped, correctly handles the race condition, and only skips an erroneous initial sync that was clobbering URL params.

The single changed file makes a minimal, well-targeted fix. The logic is sound: urlHasPosition correctly identifies the case where applyInitialUrlState() starts an async flyTo, and the onStateChanged/moveend path reliably handles the deferred first write. The only remaining finding is a P2 edge case involving the pre-existing effectiveZoom > 2 guard interacting with the new skip logic — not a regression introduced by this PR and very low real-world impact.

No files require special attention beyond the one P2 edge case noted on src/app/event-handlers.ts.

Important Files Changed

Filename Overview
src/app/event-handlers.ts Adds urlHasPosition guard in setupUrlStateSync() to skip the immediate debouncedUrlSync() call when lat/lon/zoom are present in the initial URL — correctly prevents the 250 ms debounce from reading the default map position and overwriting URL params while a flyTo animation is in progress.

Sequence Diagram

sequenceDiagram
    participant App
    participant PanelLayout
    participant EventHandlers
    participant Map
    participant URL

    App->>PanelLayout: applyInitialUrlState()
    PanelLayout->>Map: setCenter(lat, lon, zoom)
    Map-->>Map: flyTo animation starts (async)

    App->>EventHandlers: setupUrlStateSync()
    EventHandlers->>EventHandlers: urlHasPosition = (lat/lon/zoom in initialUrlState)?

    alt urlHasPosition = false (no position params in URL)
        EventHandlers->>EventHandlers: debouncedUrlSync() fires after 250ms
        EventHandlers->>Map: getCenter() returns default (lat=20, lon=0, zoom=1)
        EventHandlers->>URL: replaceState(default position) BUG
    else urlHasPosition = true (position params present) NEW BEHAVIOUR
        EventHandlers->>EventHandlers: skip debouncedUrlSync()
        Note over Map,URL: flyTo animation runs...
        Map-->>EventHandlers: onStateChanged fires on moveend
        EventHandlers->>EventHandlers: debouncedUrlSync() fires after 250ms
        EventHandlers->>Map: getCenter() returns correct position
        EventHandlers->>URL: replaceState(correct position)
    end
Loading

Reviews (1): Last reviewed commit: "fix(url-sync): don't overwrite URL posit..." | Re-trigger Greptile

Comment thread src/app/event-handlers.ts Outdated
Comment on lines +710 to +716
const urlHasPosition =
this.ctx.initialUrlState?.lat !== undefined ||
this.ctx.initialUrlState?.lon !== undefined ||
this.ctx.initialUrlState?.zoom !== undefined;
if (!urlHasPosition) {
this.debouncedUrlSync();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Silent gap when zoom ≤ 2 with lat/lon present

In applyInitialUrlState() there is an existing guard that skips setCenter() when effectiveZoom <= 2:

if (lat !== undefined && lon !== undefined) {
  const effectiveZoom = zoom ?? this.ctx.map.getState().zoom;
  if (effectiveZoom > 2) this.ctx.map.setCenter(lat, lon, zoom); // ← skipped when zoom≤2
}

When a URL like ?lat=41&lon=0&zoom=1 is loaded:

  1. urlHasPosition is true (lat/lon/zoom are all defined), so debouncedUrlSync() is correctly skipped by the new guard.
  2. However, because effectiveZoom <= 2, setCenter() is never called — no flyTo animation is started, and moveend never fires.
  3. onStateChanged therefore never fires from the animation, meaning the first URL write is silently dropped. The URL retains the original params indefinitely until the user manually interacts with the map.

Before this PR the old debouncedUrlSync() call would at least update the URL to the actual map state (even if with the wrong position). After this PR the URL is preserved but permanently out of sync with the map's actual position until the next user interaction.

The practical impact is low — a URL with zoom=1 or zoom=2 alongside explicit lat/lon is an unusual combination, and the user interacting with the map immediately restores consistency. But if precise URL-shareability on initial load is important, you may want to call debouncedUrlSync() explicitly after applyInitialUrlState() when setCenter() was skipped for this reason.

rayanhabib07 and others added 2 commits March 31, 2026 20:29
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>
…tations

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
@koala73
Copy link
Copy Markdown
Owner

koala73 commented Apr 1, 2026

Pushed a fix on top of your branch for commit 2.

setView(view, zoom?) was accepting the zoom argument but all four implementations (DeckGLMap, GlobeMap, Map, MapContainer) only had a single-parameter signature — the zoom was silently ignored at runtime and TypeScript would flag it as Expected 1 arguments, but got 2.

Updated all four:

  • DeckGLMap: zoom ?? preset.zoom into flyTo
  • GlobeMap: applies the same deck.gl→altitude mapping as setCenter when zoom is provided
  • Map (SVG): zoom ?? settings.zoom for the SVG map zoom state
  • MapContainer: forwards the optional zoom to all underlying implementations

?view=global&zoom=1.00 will now target zoom 1.00 during flyTo rather than the preset default, so moveend writes the correct value back to the URL.

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)
@koala73
Copy link
Copy Markdown
Owner

koala73 commented Apr 1, 2026

Pushed two more fixes:

P1 — SVG/mobile path dropped the zoom (MapContainer.ts:354)
this.svgMap?.setView(view) was missing the zoom forward. Fixed to setView(view, zoom).

P2 — partial URL params skipped canonicalization
urlHasPosition fired on any single param including lone ?lat or ?lon. Since applyInitialUrlState only calls setCenter when BOTH are present, a malformed ?lat=41 would suppress the initial sync and leave the stale param in the URL until the user moved the map.

Renamed to urlHasAsyncFlyTo and tightened the condition to only suppress the sync when applyInitialUrlState will actually trigger an async flyTo: view set, both lat AND lon set, or zoom set. Single-param malformed links fall through to immediate canonicalization as before.

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
@koala73
Copy link
Copy Markdown
Owner

koala73 commented Apr 1, 2026

Pushed the P1 fix.

Removed view from the urlHasAsyncFlyTo suppression guard entirely. Root cause: 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. Suppressing it was wrong for two 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.
  • GlobeMap: onStateChanged() is a no-op stub — suppressing the initial debounce leaves /?view=X deep links without any URL sync on the globe renderer.

The guard now only suppresses in the two cases where an in-flight flyTo makes getCenter() return stale intermediate coordinates:

  • lat + lon pair present → setCenter() flyTo (requires both, per the existing applyInitialUrlState logic)
  • bare zoom without a view preset → setZoom() animated zoom

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.
@koala73
Copy link
Copy Markdown
Owner

koala73 commented Apr 1, 2026

Pushed the DeckGL center fix.

DeckGLMap.setView() started a flyTo without updating this.state.zoom or caching the target center. At 250ms the debounce read:

  • state.zoom — old cached value (wrong zoom)
  • getCenter()maplibreMap.getCenter() mid-animation → intermediate coordinates

Added pendingCenter: { lat, lon } | null field. At the top of setView(), before flyTo starts:

  • this.state.zoom = zoom ?? preset.zoom (zoom correct immediately)
  • this.pendingCenter = { lat: preset.latitude, lon: preset.longitude } (center correct immediately)
  • getCenter() returns pendingCenter when set, falls through to live maplibre coords after moveend clears it

Any URL sync that fires during the flyTo — whether the initial 250ms debounce or an onStateChanged from the synchronous state write — now reads the correct destination values rather than stale/intermediate ones.

koala73 added 2 commits April 1, 2026 10:10
…+ 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.
@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 1, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
worldmonitor Ready Ready Preview, Comment Apr 1, 2026 6:15am

Request Review

@koala73 koala73 merged commit b2a0fcc into koala73:main Apr 1, 2026
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

trust:caution Brin: contributor trust score caution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants