Follow-up: notebook style review fixes (#1620)

* Address follow-up review feedback on #1619

Second round of bot review comments on #1619, which merged before they could
be addressed on that branch.

- docs/notebook.md: drop the `layer_id`-taking calls from the snippet instead
  of annotating them. The fire-and-forget client can never produce an id, so
  the example was self-contradictory; the calls are now described in prose
  below it, pointing at the widget client that does return ids.
- console_api.py: give `load_geojson` the same `**style` kwargs as
  `add_geojson`, so both console entry points style in one call.
- tests/scripting-style-params.test.ts: set the history coalesce window to 0
  so the empty-style guard is actually under test (at the app's default window
  the two writes merge and the assertion passed either way — verified by
  mutating the guard), and add a case pinning the app-window behavior: a
  styled add coalesces into one undo entry, so one Ctrl+Z removes the layer
  rather than leaving an unstyled one behind.

* Address review feedback

- Capture the app's history coalesce window with `getHistoryCoalesceMs()` and
  restore that, instead of hard-coding 400 in the cleanup path and in the
  coalescing case, so the test follows the source default if it moves.

* Address Claude review feedback

- Document the `**style` kwargs on `load_geojson` in the Python console API
  table; the signature gained them in 6260a02 but the row still read
  `load_geojson(url, name=)`, inconsistent with the `add_geojson` row above it.

Author: giswqs

apps/geolibre-desktop/src/lib/pyodide/console_api.py

@@ -197,8 +197,11 @@ def add_geojson(self, data, name="GeoJSON", **style):
         fc = _coerce_featurecollection(data)
         return _js.addGeoJsonLayer(_to_js({"name": name, "geojson": fc, "style": style}))
 
-    async def load_geojson(self, url, name="GeoJSON"):
-        """Fetch a GeoJSON URL and add it as a layer (async). Returns the id."""
+    async def load_geojson(self, url, name="GeoJSON", **style):
+        """Fetch a GeoJSON URL and add it as a layer (async). Returns the id.
+
+        Takes the same style overrides as :meth:`add_geojson`.
+        """
         from pyodide.http import pyfetch
 
         response = await pyfetch(url)
@@ -207,7 +210,7 @@ async def load_geojson(self, url, name="GeoJSON"):
         if not response.ok:
             raise RuntimeError(f"Failed to fetch {url!r}: HTTP {response.status}")
         fc = _coerce_featurecollection(await response.json())
-        return _js.addGeoJsonLayer(_to_js({"name": name, "geojson": fc}))
+        return _js.addGeoJsonLayer(_to_js({"name": name, "geojson": fc, "style": style}))
 
     def remove_layer(self, layer_id):
         """Remove a layer by id."""

docs/notebook.md

@@ -36,11 +36,6 @@ m.add_geojson(
 )  # GeoDataFrame, dict, or JSON string
 m.fit_bounds([-123, 37, -122, 38])
 m.set_basemap("https://…/style.json")
-
-# Layer-targeted calls take a layer id. This client is fire-and-forget, so
-# `add_geojson` does not hand one back — see the note below.
-m.set_visibility(layer_id, False)
-m.remove_layer(layer_id)
 ```
 
 Calls are **fire-and-forget**: each posts a command to the host app over the
@@ -51,9 +46,13 @@ client source: `backend/geolibre_server/notebook_client.py`.
 
 > Read-back queries (e.g. `get_center`) are not exposed by this fire-and-forget
 > client; they need the blocking request/reply path the `geolibre` widget uses.
-> Layer ids come from the same path: this client's `add_geojson` returns
-> `None`, while the widget's ([`geolibre` package](python.md)) returns the new
-> layer's id — which is what the id-taking calls above expect.
+
+The client also has layer-targeted calls — `set_visibility(layer_id, visible)`,
+`set_opacity`, `set_style`, `remove_layer`, `zoom_to_layer` — but they are left
+out of the snippet above because there is no way to obtain a `layer_id` here:
+being fire-and-forget, this client's `add_geojson` returns `None`. Use them with
+an id from the blocking [`geolibre` widget](python.md), whose `add_geojson`
+returns the new layer's id.
 
 ## Driving the map from an external client (VS Code, …)
 

docs/user-guide/python-console.md

@@ -124,7 +124,7 @@ is available this way.
 | `set_basemap(url)` | Set the basemap style (an http(s) or root-relative URL). |
 | `identify(lng, lat, layer_id=None)` | Query rendered features at a point (like a click). |
 | `add_geojson(data, name=, **style)` | Add a layer from a GeoJSON dict / geometry / `__geo_interface__`, with optional inline style overrides; returns the layer id. |
-| `await load_geojson(url, name=)` | Fetch a GeoJSON URL and add it; returns the layer id. |
+| `await load_geojson(url, name=, **style)` | Fetch a GeoJSON URL and add it, with the same optional inline style overrides; returns the layer id. |
 | `layers` | List of [`Layer`](#layer) objects, in draw order. |
 | `get_layer(layer_id)` | The `Layer` with that id (raises if absent). |
 | `remove_layer(layer_id)` | Remove a layer by id. |

tests/scripting-style-params.test.ts

@@ -1,6 +1,7 @@
 import assert from "node:assert/strict";
-import { beforeEach, describe, it } from "node:test";
+import { afterEach, beforeEach, describe, it } from "node:test";
 import { useAppStore } from "@geolibre/core";
+import { getHistoryCoalesceMs, setHistoryCoalesceMs } from "../packages/core/src/history";
 import { styleParamPatch } from "../apps/geolibre-desktop/src/lib/scripting/style-params";
 
 // The notebook client's `add_geojson(gdf, **style)` always sends a `style`
@@ -39,27 +40,61 @@ describe("styleParamPatch", () => {
   });
 });
 
+// Captured before any test changes it, so the app's real window is what the
+// coalescing case exercises and what cleanup restores — not a copy of it that
+// would go stale if the default moved.
+const APP_COALESCE_MS = getHistoryCoalesceMs();
+
+/** What the `addGeoJsonLayer` handler does with a raw `style` param. */
+function addGeoJsonCommand(name: string, style: unknown): string {
+  const layerId = useAppStore.getState().addGeoJsonLayer(name, FC);
+  const patch = styleParamPatch(style);
+  if (patch) useAppStore.getState().setLayerStyle(layerId, patch);
+  return layerId;
+}
+
 describe("addGeoJsonLayer command styling", () => {
   beforeEach(() => {
+    // 0 so each store write is its own history entry: with the app's default
+    // coalesce window the two writes of a styled add would merge and the guard
+    // below would pass whether or not it works.
+    setHistoryCoalesceMs(0);
     useAppStore.getState().newProject({ name: "Scripting" });
     useAppStore.temporal.getState().clear();
   });
 
+  afterEach(() => {
+    setHistoryCoalesceMs(APP_COALESCE_MS);
+  });
+
   it("merges an inline style into the new layer", () => {
-    const layerId = useAppStore.getState().addGeoJsonLayer("Styled", FC);
-    const style = styleParamPatch({ fillColor: "#facc15", strokeColor: "#d97706" });
-    assert.ok(style);
-    useAppStore.getState().setLayerStyle(layerId, style);
+    const layerId = addGeoJsonCommand("Styled", {
+      fillColor: "#facc15",
+      strokeColor: "#d97706",
+    });
 
     const layer = useAppStore.getState().layers.find((item) => item.id === layerId);
     assert.equal(layer?.style?.fillColor, "#facc15");
     assert.equal(layer?.style?.strokeColor, "#d97706");
   });
 
-  it("costs a single undo step when no style kwargs were passed", () => {
-    const layerId = useAppStore.getState().addGeoJsonLayer("Plain", FC);
-    const style = styleParamPatch({});
-    if (style) useAppStore.getState().setLayerStyle(layerId, style);
+  it("writes nothing extra when no style kwargs were passed", () => {
+    const layerId = addGeoJsonCommand("Plain", {});
+
+    assert.equal(useAppStore.temporal.getState().pastStates.length, 1);
+    useAppStore.temporal.getState().undo();
+    assert.equal(
+      useAppStore.getState().layers.find((item) => item.id === layerId),
+      undefined,
+    );
+  });
+
+  it("coalesces a styled add into one undo step at the app's window", () => {
+    // The two writes land in the same tick, so the leading debounce in the
+    // store's `handleSet` records only the first: one Ctrl+Z removes the
+    // styled layer outright rather than leaving an unstyled one behind.
+    setHistoryCoalesceMs(APP_COALESCE_MS);
+    const layerId = addGeoJsonCommand("Styled", { fillColor: "#facc15" });
 
     assert.equal(useAppStore.temporal.getState().pastStates.length, 1);
     useAppStore.temporal.getState().undo();