Address review feedback (round 4)

- Register a restored loader synchronously and fetch the service metadata
  lazily on its first query, memoized with the memo cleared on failure. A
  metadata fetch that failed once left the layer permanently unbound, with
  refresh reporting a transient-sounding error that nothing ever retried;
  now the next pan or refresh retries it, and there is no window in which a
  refresh finds the layer unbound at all.
- Reword the refresh error accordingly: no loader now means no map, not a
  race with startup.
- Drop the unreachable abort branch in fetchArcGISFeatureCount — a viewport
  query is the only cancellable caller and planArcGISPaging skips the count
  for those, so the signal was always undefined where it ran.
- Test restoreArcGISViewportLayers end to end: synchronous binding, a failed
  metadata read reported and retried on the next pan, rebinding when the map
  instance changes, and reloadArcGISViewportLayer re-querying the viewport.

Author: giswqs

apps/geolibre-desktop/src/lib/layer-refresh.ts

@@ -579,12 +579,12 @@ async function refreshArcGISLayer(layer: GeoLibreLayer): Promise<GeoJsonRefreshR
     await import("@geolibre/plugins");
   if (layer.metadata.viewportLoading === true) {
     const viewport = reloadArcGISViewportLayer(layer.id);
-    // No live loader yet: a just-reopened project is still resolving the
-    // service metadata its loader needs, or that resolve failed. Falling
-    // through to the unbounded replay below would download the entire service
-    // — the cost this layer is loaded by viewport to avoid — so say so instead.
+    // No loader at all: the layer is in a host with no map (`restoreArcGISViewportLayers`
+    // registers one synchronously wherever there is one). Falling through to
+    // the unbounded replay below would download the entire service — the cost
+    // this layer is loaded by viewport to avoid — so say so instead.
     if (!viewport) {
-      throw new Error("This layer is still binding to the map viewport. Try again in a moment.");
+      throw new Error("This layer is not bound to a map viewport, so it cannot be refreshed.");
     }
     const bounded = await viewport;
     return { geojson: bounded, featureCount: bounded.features.length };

packages/plugins/src/plugins/arcgis-layer.ts

@@ -445,17 +445,23 @@ async function addArcGISFeatureLayerAsGeoJson(
 
   const bounds = arcgisExtentToBounds(layerInfo.extent);
   if (bounds && options.zoomTo !== false) app.fitBounds?.(bounds);
-  if (map) startArcGISViewportLoader(id, map, queryUrl, options, layerInfo);
+  if (map) startArcGISViewportLoader(id, map, queryUrl, options, () => Promise.resolve(layerInfo));
   return id;
 }
 
-/** Keep one FeatureServer layer synchronized with the settled map viewport. */
+/**
+ * Keep one FeatureServer layer synchronized with the settled map viewport.
+ *
+ * `resolveLayerInfo` is a thunk rather than a value so the restore path can
+ * register a loader before it has the service metadata: the fetch then happens
+ * on the first query, and a failed one is retried by the next.
+ */
 function startArcGISViewportLoader(
   layerId: string,
   map: maplibregl.Map,
   queryUrl: string,
   options: ArcGISLayerOptions,
-  layerInfo: ArcGISFeatureLayerInfo,
+  resolveLayerInfo: () => Promise<ArcGISFeatureLayerInfo>,
 ): void {
   let abort: AbortController | null = null;
   let requestSequence = 0;
@@ -488,6 +494,13 @@ function startArcGISViewportLoader(
     abort = controller;
     loader.abort = controller;
     const sequence = ++requestSequence;
+    let layerInfo: ArcGISFeatureLayerInfo;
+    try {
+      layerInfo = await resolveLayerInfo();
+    } catch (error) {
+      if (sequence !== requestSequence) return currentArcGISLayerGeojson(layerId);
+      throw error;
+    }
     const envelopes = arcgisViewportEnvelopes(map.getBounds());
     // One bucket per envelope, so a viewport split across the antimeridian
     // publishes both halves together instead of each replacing the other.
@@ -579,6 +592,10 @@ export function reloadArcGISViewportLayer(layerId: string): Promise<FeatureColle
  * view when it was saved: panning fetches nothing, and a refresh falls back to
  * the unbounded download the viewport path exists to avoid.
  *
+ * Loaders are registered synchronously, before the service metadata they need
+ * is fetched, so there is no window in which a refresh finds the layer
+ * unbound.
+ *
  * @param app - The host app API, for the map the loaders bind to.
  */
 export function restoreArcGISViewportLayers(app: GeoLibreAppAPI): void {
@@ -608,21 +625,23 @@ export function restoreArcGISViewportLayers(app: GeoLibreAppAPI): void {
       pageSize: typeof source.pageSize === "number" ? source.pageSize : undefined,
       sourceType: "url",
     };
-    const layerId = layer.id;
     // Re-read the service metadata rather than trusting a stored copy, the same
     // reason refreshArcGISFeatureLayer does: paging capabilities and
-    // `maxRecordCount` are the service's to change between sessions.
-    void fetchArcGISJson<ArcGISFeatureLayerInfo>(
-      trimTrailingSlash(queryUrl).replace(/\/query$/i, ""),
-      options,
-      undefined,
-    )
-      .then((layerInfo) => {
-        // The layer may have been removed while the metadata was in flight.
-        if (!useAppStore.getState().layers.some((entry) => entry.id === layerId)) return;
-        startArcGISViewportLoader(layerId, map, queryUrl, options, layerInfo);
-      })
-      .catch((error: unknown) => handleArcGISViewportError(layerId, error));
+    // `maxRecordCount` are the service's to change between sessions. Fetched
+    // lazily on the first query and memoized, with a failure clearing the
+    // memo — so a blocked or flaky reopen retries on the next pan or refresh
+    // instead of leaving the layer permanently unbound.
+    let pending: Promise<ArcGISFeatureLayerInfo> | null = null;
+    const resolveLayerInfo = (): Promise<ArcGISFeatureLayerInfo> =>
+      (pending ??= fetchArcGISJson<ArcGISFeatureLayerInfo>(
+        trimTrailingSlash(queryUrl).replace(/\/query$/i, ""),
+        options,
+        undefined,
+      ).catch((error: unknown) => {
+        pending = null;
+        throw error;
+      }));
+    startArcGISViewportLoader(layer.id, map, queryUrl, options, resolveLayerInfo);
   }
 }
 
@@ -1241,7 +1260,7 @@ async function planArcGISPaging(
     supportsOrderBy: layerInfo.advancedQueryCapabilities?.supportsOrderBy !== false,
     // Spatial counts can be as expensive as fetching the first page on large
     // polygon services. Start rendering immediately for viewport queries.
-    total: request.params ? null : await fetchArcGISFeatureCount(queryUrl, params, request.signal),
+    total: request.params ? null : await fetchArcGISFeatureCount(queryUrl, params),
   };
 }
 
@@ -1275,13 +1294,16 @@ function positiveInteger(value: number | undefined): number | null {
  * condition. Best-effort: a service that will not answer `returnCountOnly`
  * still pages fine, so any failure resolves to `null` rather than throwing.
  *
+ * Takes no abort signal, and needs none: {@link planArcGISPaging} skips the
+ * count entirely for a viewport query (the only cancellable caller), because a
+ * spatial count can cost as much as the first page.
+ *
  * @param queryUrl - The layer's `/query` endpoint.
- * @param token - The access token to send, if any.
+ * @param params - The query params every request in the plan shares.
  */
 async function fetchArcGISFeatureCount(
   queryUrl: string,
   params: Record<string, string | undefined>,
-  signal?: AbortSignal,
 ): Promise<number | null> {
   try {
     const response = await fetch(
@@ -1291,15 +1313,13 @@ async function fetchArcGISFeatureCount(
         returnCountOnly: "true",
         where: "1=1",
       }),
-      { signal },
     );
     if (!response.ok) return null;
     const json = (await response.json()) as { count?: unknown };
     return typeof json.count === "number" && Number.isFinite(json.count) && json.count >= 0
       ? json.count
       : null;
-  } catch (error) {
-    if (signal?.aborted) throw error;
+  } catch {
     return null;
   }
 }

tests/arcgis-feature-layer.test.ts

@@ -5,6 +5,8 @@ import type { GeoLibreAppAPI } from "../packages/plugins/src/types";
 import {
   addArcGISLayer,
   refreshArcGISFeatureLayer,
+  reloadArcGISViewportLayer,
+  restoreArcGISViewportLayers,
 } from "../packages/plugins/src/plugins/arcgis-layer";
 
 // Minimal ArcGIS FeatureServer layer metadata (the `?f=json` response) with a
@@ -461,6 +463,76 @@ describe("addArcGISLayer (feature layer)", () => {
     assert.equal(layer?.geojson?.features.length, 1, "the shared feature is published once");
   });
 
+  it("re-binds a reopened project's layer to the viewport", async () => {
+    const geometries: string[] = [];
+    let metadataFailures = 1;
+    globalThis.fetch = (async (input: RequestInfo | URL) => {
+      const url = new URL(typeof input === "string" ? input : input.toString());
+      if (!url.pathname.endsWith("/query")) {
+        // The first metadata read fails, as a flaky reopen would.
+        if (metadataFailures > 0) {
+          metadataFailures -= 1;
+          throw new Error("Service unavailable");
+        }
+        return jsonResponse(VIEWPORT_LAYER_INFO);
+      }
+      const offset = Number(url.searchParams.get("resultOffset") ?? "0");
+      if (offset > 0) return jsonResponse({ type: "FeatureCollection", features: [] });
+      geometries.push(url.searchParams.get("geometry") ?? "");
+      return jsonResponse({ type: "FeatureCollection", features: [viewportFeature(1)] });
+    }) as typeof fetch;
+
+    // A layer as a saved project restores it: viewport metadata and the stored
+    // query URL, but no live loader.
+    const id = useAppStore
+      .getState()
+      .addGeoJsonLayer("Restored", { type: "FeatureCollection", features: [] }, undefined, null);
+    useAppStore.getState().updateLayer(id, {
+      source: { type: "geojson", arcgisQueryUrl: `${SERVICE_URL}/query` },
+      metadata: { sourceKind: "arcgis-feature-query", viewportLoading: true },
+    });
+    assert.equal(reloadArcGISViewportLayer(id), null, "no loader before the project is restored");
+
+    const view = fakeViewportMap([144, -39, 146, -37]);
+    restoreArcGISViewportLayers({ getMap: () => view.map } as unknown as GeoLibreAppAPI);
+    // Registered before the metadata fetch is even started, so a refresh in
+    // this window never finds the layer unbound.
+    assert.ok(view.listeners.has("moveend"), "the loader binds synchronously");
+    await settle();
+
+    // The first query failed on metadata, which is reported, not swallowed.
+    assert.match(
+      useAppStore.getState().layers.find((layer) => layer.id === id)?.connection?.lastError ?? "",
+      /Service unavailable/,
+    );
+
+    // The next pan retries the metadata rather than staying stuck.
+    view.setBounds([150, -35, 152, -33]);
+    view.listeners.get("moveend")?.();
+    await settle();
+    assert.deepEqual(geometries, ["150,-35,152,-33"]);
+    const restored = useAppStore.getState().layers.find((layer) => layer.id === id);
+    assert.equal(restored?.connection?.lastError, null);
+    assert.equal(restored?.geojson?.features.length, 1);
+
+    // Re-running the restore against a new map rebinds rather than skipping.
+    const remounted = fakeViewportMap([10, 10, 12, 12]);
+    restoreArcGISViewportLayers({ getMap: () => remounted.map } as unknown as GeoLibreAppAPI);
+    await settle();
+    assert.deepEqual(
+      view.offCalls.map(([event]) => event),
+      ["moveend"],
+      "the loader detaches from the old map",
+    );
+    assert.ok(remounted.listeners.has("moveend"));
+    assert.deepEqual(geometries, ["150,-35,152,-33", "10,10,12,12"]);
+
+    // A refresh takes this same bounded path rather than the unbounded replay.
+    const reloaded = await reloadArcGISViewportLayer(id);
+    assert.equal(reloaded?.features.length, 1);
+    assert.deepEqual(geometries, ["150,-35,152,-33", "10,10,12,12", "10,10,12,12"]);
+  });
+
   it("holds a split viewport to maxFeatures across both envelopes", async () => {
     // Each half answers with the cap's worth of distinct features, so an
     // uncoordinated limit would leave the layer holding twice the maximum.