fix(basemaps): make the basemap control's Mapbox styles load (#1488)

* fix(basemaps): make the basemap control's Mapbox styles load

Every Mapbox basemap in the basemap control produced
`name: unknown property "name"` and a blank map.

The control applies a Mapbox style correctly — it passes MapLibre a
`transformStyle` that rewrites the `mapbox://` sprite/glyphs/source URLs
and swaps Mapbox's `projection: { name }` for the spec's `{ type }`, plus
`validate: false`. But GeoLibre is store-driven: the change is written to
`basemapStyleUrl` and MapController re-applies it with a plain
`map.setStyle(url)`, which drops both. MapLibre's validator then rejects
`projection.name`, and `Style._load` returns on the first validation
error, so the whole style is abandoned. The three styles with no
`projection` block (satellite-v9, navigation-day/night-v1) passed
validation but still rendered empty, since nothing resolves `mapbox://`.

Teach GeoLibre's own style path about Mapbox instead of relying on the
control's: `mapbox-style.ts` fetches the descriptor and converts it, and
MapController applies the result. That also covers the paths the control
is not part of — a project reopened with a Mapbox basemap, and a
split-view pane, both of which build a MapController straight from the
saved URL. The token comes back out of the URL's `access_token`
parameter, so no credential plumbing is needed to *apply* a style and a
saved project stays self-contained. A generation counter drops a
descriptor whose basemap the user has already switched away from.

Also wire `MAPBOX_TOKEN` / `VITE_MAPBOX_ACCESS_TOKEN` through to the
control's `mapboxAccessToken`, matching the bare→prefixed bridge the
Google Maps and Cesium keys use, so the panel's API-keys field is
pre-filled instead of demanding a paste. Pushed only when set, like the
other panel-enterable providers.

Verified in the browser against all of Streets/Satellite in single and
split view: sprite, fonts, TileJSON and tiles all 200, zero console
errors, and non-Mapbox styles still switch cleanly afterwards.

* Address CodeRabbit review feedback

- Don't wedge the controller when the Mapbox descriptor fetch fails.
  setStyle tore down styleReady/paint values/the layer control up front,
  but the Mapbox path only reaches map.setStyle after an async fetch — a
  rejection left no style.load coming to rebuild any of it, so layer
  syncing, basemap visibility/opacity and the layer control stayed dead
  over a still-rendered old style. Teardown now happens in beginStyleSwap
  immediately before each map.setStyle, so a failed fetch changes nothing.
  Verified by routing the descriptor to a 500: the previous style keeps
  rendering, all six control groups survive, and the next basemap switch
  applies normally.

- Don't log the token-bearing style URL. The URL carries the user's
  access_token, so the failure warning now logs a redacted descriptor id
  via redactMapboxStyleUrl (origin + path, no query), covered by tests.

- Test that the access_token is trimmed when the query string pads it.

- Test that transformMapboxStyle leaves the nested source URL alone, not
  just the top-level sprite/projection — an in-place rewrite would have
  passed the old assertions.

- Document the prefixed-over-bare precedence on getMapboxAccessToken. A
  Settings entry overrides a baked token under the same VITE_ name; the
  bare MAPBOX_TOKEN is a fallback for when nothing was baked in, not an
  override. The docstring implied otherwise.

* Address CodeRabbit review feedback

- Abort a superseded Mapbox descriptor request. The generation counter
  already kept a stale style from being applied, but the fetch itself
  kept running; a rapid run of basemap changes (or a request that never
  settles) left several in flight, each holding a controller closure
  alive. applyStyleToMap now keeps an AbortController for the active
  load, aborts it when starting a newer generation and from destroy(),
  and passes its signal to loadMapboxStyle — which already accepted one,
  so this also retires a parameter that was left unwired.

  An abort rejects with AbortError, but only ever on a generation that
  has already been superseded (abort follows the ++ in applyStyleToMap,
  and destroy() nulls the map first), so both handlers return before the
  warning — a cancelled request is never reported as a failure.

* Cover the Mapbox descriptor abort with tests

93fe1aa wired an AbortController into applyStyleToMap but left the
behavior untested. Adds three cases against a stub map and a fetch that
never settles on its own:

- a newer Mapbox basemap aborts the request it supersedes, and the abort
  rejection produces no failed-style console warning,
- a switch to a plain (non-Mapbox) style URL aborts the pending request
  too, and applies synchronously without a second fetch,
- destroy() aborts a request still in flight.

Author: giswqs

apps/geolibre-desktop/vite.config.ts

@@ -76,6 +76,19 @@ if (!process.env.VITE_CESIUM_TOKEN) {
   }
 }
 
+// Mapbox access token for the basemap control's Mapbox styles: same
+// bare→prefixed bridge as the Google Maps and Cesium keys. `MAPBOX_TOKEN` is the
+// spelling Mapbox's own tooling uses, so accept it from the shell or an .env
+// file and surface it as `VITE_MAPBOX_ACCESS_TOKEN`; getMapboxAccessToken() then
+// lets a runtime Settings override win over this build-time value.
+if (!process.env.VITE_MAPBOX_ACCESS_TOKEN) {
+  const mapboxAccessToken =
+    process.env.MAPBOX_TOKEN || FILE_ENV.VITE_MAPBOX_ACCESS_TOKEN || FILE_ENV.MAPBOX_TOKEN;
+  if (mapboxAccessToken) {
+    process.env.VITE_MAPBOX_ACCESS_TOKEN = mapboxAccessToken;
+  }
+}
+
 // Earth Engine OAuth client ID: same bare→prefixed bridge as the Google Maps
 // and Cesium keys. The app reads `import.meta.env.VITE_GEE_OAUTH_CLIENT_ID`, so
 // a bare `GEE_OAUTH_CLIENT_ID` (shell/.zshrc or an .env file) is surfaced under

packages/core/src/index.ts

@@ -91,6 +91,7 @@ export {
 export {
   getCesiumIonToken,
   getGoogleMapsApiKey,
+  getMapboxAccessToken,
   getProtomapsApiKey,
   getProtomapsStyleUrl,
   getRuntimeEnvironment,

packages/core/src/runtime-env.ts

@@ -90,6 +90,36 @@ export function getGoogleMapsApiKey(env?: Record<string, string | undefined>): s
   return trimmed || undefined;
 }
 
+/**
+ * Resolves the Mapbox access token from the runtime environment.
+ *
+ * The basemap control's Mapbox styles authenticate with the user's own token.
+ * It is supplied via `VITE_MAPBOX_ACCESS_TOKEN` (baked in at build time from the
+ * bare `MAPBOX_TOKEN` env var, which `vite.config.ts` copies into the prefixed
+ * name — the spelling Mapbox's own tooling uses) or set at runtime through
+ * Settings → Environment variables (`window.__GEOLIBRE_RUNTIME_ENV__`, which
+ * bypasses Vite's envPrefix allowlist, so a bare `MAPBOX_TOKEN` entry works
+ * there too). When unset, the Mapbox basemaps prompt for a token in the basemap
+ * panel's API keys view instead.
+ *
+ * Note the precedence, shared with {@link getGoogleMapsApiKey} and
+ * {@link getCesiumIonToken}: the prefixed name always wins over the bare one,
+ * and `getRuntimeEnvironment` merges build-time and runtime vars into one
+ * record. So a build that baked in `VITE_MAPBOX_ACCESS_TOKEN` is overridden at
+ * runtime by a Settings entry under that *same* prefixed name; a bare
+ * `MAPBOX_TOKEN` entry is a fallback for when nothing was baked in, not a way
+ * to override a baked token.
+ *
+ * @param env - Environment record (defaults to the runtime environment);
+ *   injectable for testing.
+ * @returns The trimmed token, or undefined when unset.
+ */
+export function getMapboxAccessToken(env?: Record<string, string | undefined>): string | undefined {
+  const runtimeEnv = env ?? getRuntimeEnvironment();
+  const trimmed = runtimeEnv.VITE_MAPBOX_ACCESS_TOKEN?.trim() || runtimeEnv.MAPBOX_TOKEN?.trim();
+  return trimmed || undefined;
+}
+
 /**
  * Resolves the Cesium Ion access token from the runtime environment.
  *

packages/map/src/index.ts

@@ -53,6 +53,14 @@ export {
   type ProtomapsFlavor,
   type ProtomapsBasemapStyleOptions,
 } from "./protomaps-basemap";
+export {
+  isMapboxStyleUrl,
+  loadMapboxStyle,
+  mapboxAccessTokenFromStyleUrl,
+  redactMapboxStyleUrl,
+  resolveMapboxInternalUrl,
+  transformMapboxStyle,
+} from "./mapbox-style";
 export {
   ensureRemotePMTilesArchive,
   hasPMTilesArchive,

packages/map/src/map-controller.ts

@@ -40,6 +40,7 @@ import {
   vectorTileStyleLayerIds,
 } from "./layer-sync";
 import { installGlobePopupOcclusion } from "./globe-popup-occlusion";
+import { isMapboxStyleUrl, loadMapboxStyle, redactMapboxStyleUrl } from "./mapbox-style";
 import { PlanetaryScaleControl } from "./planetary-scale-control";
 import { getOfflineBasemapStyle, isOfflineBasemapSentinel } from "./protomaps-basemap";
 import { ResetBearingControl } from "./reset-bearing-control";
@@ -380,6 +381,12 @@ export class MapController {
   // (reentrancy guard against a sync loop). See syncLayerControlState.
   private refreshingStyleEditor = false;
   private basemapStyleUrl = DEFAULT_BASEMAP;
+  // Bumped on every style application so an asynchronously resolved style (the
+  // Mapbox path in applyStyleToMap) can tell whether it is still the current one.
+  private styleGeneration = 0;
+  // Aborts the in-flight Mapbox descriptor request, so a superseded basemap
+  // change (or destroy) does not leave the fetch running.
+  private pendingMapboxStyleAbort: AbortController | null = null;
   private basemapVisible = true;
   private basemapOpacity = 1;
   private mapPreferences: MapPreferences = DEFAULT_PROJECT_PREFERENCES.map;
@@ -422,9 +429,14 @@ export class MapController {
     const maxPitch = clampNumber(mapPreferences.maxPitch, 0, DEFAULT_MAX_PITCH);
     this.mapPreferences = mapPreferences;
     this.basemapStyleUrl = options.styleUrl ?? DEFAULT_BASEMAP;
+    // A Mapbox descriptor has to be fetched and rewritten before MapLibre will
+    // take it (see applyStyleToMap), which the Map constructor cannot wait for.
+    // Start blank and apply it below, as soon as the listeners are wired — the
+    // path a project saved with a Mapbox basemap and a split-view pane both take.
+    const deferMapboxStyle = isMapboxStyleUrl(this.basemapStyleUrl);
     this.map = new maplibregl.Map({
       container,
-      style: resolveMapStyle(this.basemapStyleUrl),
+      style: deferMapboxStyle ? createBlankMapStyle() : resolveMapStyle(this.basemapStyleUrl),
       center: view?.center ?? [-100, 40],
       zoom: view?.zoom ?? 2,
       bearing: view?.bearing ?? 0,
@@ -496,6 +508,10 @@ export class MapController {
     this.addAttributionControl();
     this.addLogoControl();
     this.addMaptoolkitLogoControl();
+    // Kick off the deferred Mapbox descriptor fetch now that `style.load` and
+    // `styledata` are wired, so the real style is treated exactly like a later
+    // basemap switch rather than racing the listeners above.
+    if (deferMapboxStyle) this.applyStyleToMap(this.basemapStyleUrl);
     return this.map;
   }
 
@@ -863,6 +879,7 @@ export class MapController {
       clearTimeout(this.layerControlStyleRefreshTimer);
       this.layerControlStyleRefreshTimer = null;
     }
+    this.abortPendingMapboxStyle();
     this.map?.remove();
     this.map = null;
     this.styleReady = false;
@@ -872,16 +889,99 @@ export class MapController {
   setStyle(url: string): void {
     if (!this.map) return;
     this.basemapStyleUrl = url;
-    this.styleReady = false;
-    this.basemapOriginalPaintValues.clear();
-    this.removeLayerControl();
-    this.map.setStyle(resolveMapStyle(url));
+    this.applyStyleToMap(url);
     // Switching to/from a planetary basemap changes the active body (the store's
     // ellipsoid subscription runs first, so the singleton is already current),
     // so redraw the scale bar for the new radius without waiting for a pan.
     this.scaleControl?.refresh();
   }
 
+  /**
+   * Tears down the state that belongs to the outgoing style, immediately before
+   * the incoming one is handed to MapLibre. `style.load` rebuilds all of it.
+   *
+   * Deliberately called per style application rather than at the top of
+   * `setStyle`: the Mapbox path below only reaches `map.setStyle` after an
+   * asynchronous fetch, and tearing down first would leave the controller
+   * wedged if that fetch failed — `styleReady` stuck false with no `style.load`
+   * coming to clear it, so layer syncing, basemap visibility/opacity and the
+   * layer control would all stay disabled over a still-rendered old style.
+   */
+  private beginStyleSwap(): void {
+    this.styleReady = false;
+    this.basemapOriginalPaintValues.clear();
+    this.removeLayerControl();
+  }
+
+  /**
+   * Hands a basemap style URL to MapLibre.
+   *
+   * Everything except Mapbox resolves synchronously, so `setStyle` is handed the
+   * URL (or the inline style a GeoLibre sentinel expands to) directly. Mapbox
+   * style descriptors are Mapbox-flavored and must be fetched and rewritten
+   * before MapLibre will accept them (see ./mapbox-style), which makes that path
+   * asynchronous: a generation counter drops the result of a swap the user has
+   * already superseded, so a slow descriptor can never overwrite a newer
+   * basemap. Validation is off for those, matching how the basemap control
+   * applies them — the descriptor is transformed to spec, not authored here.
+   */
+  private applyStyleToMap(url: string): void {
+    const map = this.map;
+    if (!map) return;
+    const generation = ++this.styleGeneration;
+    // Drop any descriptor still in flight for the basemap this one replaces:
+    // its result is already destined for the generation check below, so the
+    // request is pure waste. Also covers a request that never settles, which
+    // would otherwise keep its closure (and this controller) alive.
+    this.abortPendingMapboxStyle();
+
+    if (!isMapboxStyleUrl(url)) {
+      this.beginStyleSwap();
+      map.setStyle(resolveMapStyle(url));
+      return;
+    }
+
+    const pending = new AbortController();
+    this.pendingMapboxStyleAbort = pending;
+    void loadMapboxStyle(url, pending.signal)
+      .then((style) => {
+        if (this.map !== map || this.styleGeneration !== generation) return;
+        this.beginStyleSwap();
+        map.setStyle(style, { validate: false });
+      })
+      .catch((error: unknown) => {
+        if (this.map !== map || this.styleGeneration !== generation) return;
+        // Nothing was torn down (beginStyleSwap runs only on success), so the
+        // controller stays fully live over the style it already had — better
+        // than blanking the map. The basemap control watches its own parallel
+        // setStyle and rolls the basemap back through the store when a provider
+        // style fails, which arrives here as a newer generation.
+        // The URL carries the user's access_token, so log the descriptor id only.
+        console.warn(
+          `Failed to load the Mapbox basemap style "${redactMapboxStyleUrl(url)}".`,
+          error,
+        );
+      })
+      .finally(() => {
+        if (this.pendingMapboxStyleAbort === pending) {
+          this.pendingMapboxStyleAbort = null;
+        }
+      });
+  }
+
+  /**
+   * Cancels an in-flight Mapbox descriptor request, if any.
+   *
+   * An abort rejects the fetch with an `AbortError`, but that rejection always
+   * lands on a superseded generation (this is only called after bumping it, or
+   * from `destroy`, which nulls the map), so the handlers above return before
+   * reaching the warning — no aborted request is ever reported as a failure.
+   */
+  private abortPendingMapboxStyle(): void {
+    this.pendingMapboxStyleAbort?.abort();
+    this.pendingMapboxStyleAbort = null;
+  }
+
   setBasemapVisible(visible: boolean): void {
     this.basemapVisible = visible;
     this.applyBasemapVisibility();