Fix pre-release defects from the v1.0.30 regression audit

src/viser/_gui_handles.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import asyncio
 import base64
 import dataclasses
 import json
@@ -72,6 +73,7 @@
     SplitPlacement,
 )
 from ._scene_api import _encode_image_binary
+from ._threadpool_exceptions import print_task_error
 from .infra import ClientId
 
 if TYPE_CHECKING:
@@ -260,7 +262,12 @@ def value(self, value: T | np.ndarray) -> None:
             # reduces the likelihood of many common race conditions.
             cb_out = cb(GuiEvent(client_id=None, client=None, target=self))
             if isinstance(cb_out, Coroutine):
-                self._impl.gui_api._event_loop.create_task(cb_out)
+                # run_coroutine_threadsafe, not create_task: assignment
+                # typically happens on a user thread, and create_task is
+                # neither thread-safe nor guaranteed to wake the loop.
+                asyncio.run_coroutine_threadsafe(
+                    cb_out, self._impl.gui_api._event_loop
+                ).add_done_callback(print_task_error)
 
     @property
     def update_timestamp(self) -> float:
@@ -387,10 +394,14 @@ def _colors_to_int_tuple(value: Any, *, warn_stacklevel: int) -> tuple[int, ...]
     assignment path needs 5."""
     if isinstance(value, np.ndarray):
         assert value.ndim == 1, f"Expected a 1D color, got shape {value.shape}."
+    # Materialize once up front: we iterate twice (the float-channel check and
+    # the tuple build below), so a generator input would arrive exhausted at
+    # the second pass and silently yield an empty/partial color.
+    value = tuple(value)
     if any(not np.issubdtype(type(v), np.integer) and v > 1.0 for v in value):
         warnings.warn(
             "Float color channels are interpreted on [0, 1] and scaled to "
-            f"[0, 255]; values > 1.0 are clamped to 255 (got {tuple(value)!r}). "
+            f"[0, 255]; values > 1.0 are clamped to 255 (got {value!r}). "
             "Use ints for absolute [0, 255] channels.",
             stacklevel=warn_stacklevel,
         )
@@ -762,8 +773,8 @@ def _rebuild_tab_props(self) -> None:
         icon change) rebuilds through here, so the parallel tuples can never
         desync in length or order."""
         if self._impl.removed:
-            # Tear-down path: PanelHandle.remove() tombstones BEFORE draining
-            # its tabs (the tombstone must be an atomic check-and-set against
+            # Tear-down path: both mixers' remove() tombstone BEFORE draining
+            # their tabs (the tombstone must be an atomic check-and-set against
             # concurrent removers), so the drain's write-backs land here after
             # removal. Skip the wire write-back: props_setattr would
             # (correctly) reject a props write on a removed handle, and the
@@ -792,27 +803,29 @@ def __post_init__(self) -> None:
 
     def remove(self) -> None:
         """Remove this tab group and all contained GUI elements."""
-        # Warn if already removed.
-        if self._impl.removed:
-            warnings.warn(
-                f"Attempted to remove an already removed {self.__class__.__name__}.",
-                stacklevel=2,
-            )
-            return
-
-        # Remove tabs first. Each tab.remove() writes back to this group's
-        # tab-list props (_tab_labels / _tab_icons_html / _tab_container_ids), so
-        # we must NOT mark the group removed until afterwards -- otherwise the
-        # removed-handle guard in props_setattr raises on those writes, leaving
-        # the group half-removed (still in its parent's _children with
-        # removed=True). A subsequent gui.reset() then spins forever, since its
-        # `while root._children: child.remove()` loop hits that group whose
-        # remove() now no-ops via the already-removed guard.
+        gui_api = self._impl.gui_api
+        # Tombstone CHECK-AND-SET under the lifecycle lock, mirroring
+        # PanelHandle.remove(): add_tab() takes the same lock for its
+        # removed-check + append, so an unlocked tombstone here let a
+        # concurrent add_tab interleave between our tab snapshot and the
+        # tombstone -- registering a live container entry for a group that no
+        # longer exists, which silently accepts children forever.
+        with gui_api._panel_lifecycle_lock:
+            if self._impl.removed:
+                warnings.warn(
+                    f"Attempted to remove an already removed {self.__class__.__name__}.",
+                    stacklevel=2,
+                )
+                return
+            self._impl.removed = True
+            gui_api._websock_interface.queue_message(GuiRemoveMessage(self._impl.uuid))
+        # Only the tombstone winner reaches here. The tab drain runs AFTER the
+        # tombstone and OUTSIDE the (non-reentrant) lock: each tab.remove()
+        # writes back to this group's tab tuples, which _rebuild_tab_props
+        # skips for a removed group (props_setattr would reject the write; the
+        # client drops the whole entity via the remove message anyway).
         for tab in tuple(self._tab_handles):
             tab.remove()
-        self._impl.removed = True
-        gui_api = self._impl.gui_api
-        gui_api._websock_interface.queue_message(GuiRemoveMessage(self._impl.uuid))
         parent = gui_api._container_handle_from_uuid[self._impl.parent_container_id]
         parent._children.pop(self._impl.uuid)
 
@@ -1470,7 +1483,13 @@ def submit(self) -> None:
         for cb in self._submit_cb:
             cb_out = cb(GuiEvent(client_id=None, client=None, target=self))
             if isinstance(cb_out, Coroutine):
-                gui_api._event_loop.create_task(cb_out)
+                # run_coroutine_threadsafe, not create_task: submit() is
+                # typically called from a user thread (e.g. a button's
+                # on_click handler), and create_task is neither thread-safe
+                # nor guaranteed to wake the loop.
+                asyncio.run_coroutine_threadsafe(
+                    cb_out, gui_api._event_loop
+                ).add_done_callback(print_task_error)
         # Broadcast to clients so they reset dirty state.
         gui_api._websock_interface.queue_message(
             GuiFormSubmitMessage(uuid=self._impl.uuid)

src/viser/_scene_api.py

@@ -3276,8 +3276,11 @@ def _fire_scene_pointer_done_callbacks(self) -> None:
         # must not starve its siblings or leave the list uncleared.
         for cleanup in list(self._scene_pointer_done_cb):
             if asyncio.iscoroutinefunction(cleanup):
-                task = self._event_loop.create_task(cleanup())
-                task.add_done_callback(print_task_error)
+                # run_coroutine_threadsafe, not create_task: callback removal
+                # can run on a user thread, and create_task is neither
+                # thread-safe nor guaranteed to wake the loop.
+                future = asyncio.run_coroutine_threadsafe(cleanup(), self._event_loop)
+                future.add_done_callback(print_task_error)
             else:
                 try:
                     cleanup()

src/viser/_tunnel.py

@@ -141,10 +141,18 @@ def close(self) -> None:
         if self._thread is not None:
             assert self._event_loop is not None
 
-            @self._event_loop.call_soon_threadsafe
-            def _() -> None:
-                assert self._close_event is not None
-                self._close_event.set()
+            try:
+
+                @self._event_loop.call_soon_threadsafe
+                def _() -> None:
+                    assert self._close_event is not None
+                    self._close_event.set()
+
+            except RuntimeError:
+                # The tunnel job already tore its loop down (a failed
+                # connection exits and closes the loop); there's nothing left
+                # to signal, but we still want the join + disconnect below.
+                pass
 
             self._thread.join()
             self._disconnect_event.set()

src/viser/_viser.py

@@ -4,6 +4,7 @@
 import atexit
 import dataclasses
 import io
+import math
 import mimetypes
 import os
 import threading
@@ -376,6 +377,19 @@ def min_orbit_distance(self) -> float:
 
     @min_orbit_distance.setter
     def min_orbit_distance(self, min_orbit_distance: float) -> None:
+        # Validate BEFORE the no-op short-circuit: a bad value must raise even
+        # when it happens to be "close" to the stored one (e.g. re-assigning
+        # the same bad value after a raise).
+        if not np.isfinite(min_orbit_distance) or min_orbit_distance <= 0.0:
+            raise ValueError(
+                f"min_orbit_distance ({min_orbit_distance}) must be a "
+                "positive, finite number."
+            )
+        if min_orbit_distance > self._state.max_orbit_distance:
+            raise ValueError(
+                f"min_orbit_distance ({min_orbit_distance}) must be <= "
+                f"max_orbit_distance ({self._state.max_orbit_distance})."
+            )
         if np.allclose(self._state.min_orbit_distance, min_orbit_distance):
             return
         self._state.min_orbit_distance = min_orbit_distance
@@ -388,7 +402,8 @@ def min_orbit_distance(self, min_orbit_distance: float) -> None:
     def max_orbit_distance(self) -> float:
         """How far the camera may be dollied out from its orbit (look-at) point.
         Distinct from :attr:`far`, which clips rendering rather than camera
-        travel. Defaults to 1e4. Synchronized automatically when assigned.
+        travel. Defaults to 1e4; set to ``float("inf")`` for the unbounded
+        pre-1.1.0 behavior. Synchronized automatically when assigned.
 
         Dolly is multiplicative per wheel event, so a very large maximum means a long
         scroll — a trackpad's inertial tail, for instance — can walk the camera out to
@@ -399,6 +414,19 @@ def max_orbit_distance(self) -> float:
 
     @max_orbit_distance.setter
     def max_orbit_distance(self, max_orbit_distance: float) -> None:
+        # Validate BEFORE the no-op short-circuit; see min_orbit_distance.
+        # +inf is allowed: it is the pre-1.1.0 unbounded behavior (and the
+        # camera-controls default), the opt-out for large-scale scenes.
+        if math.isnan(max_orbit_distance) or max_orbit_distance <= 0.0:
+            raise ValueError(
+                f"max_orbit_distance ({max_orbit_distance}) must be a "
+                "positive number (or float('inf') for unbounded)."
+            )
+        if max_orbit_distance < self._state.min_orbit_distance:
+            raise ValueError(
+                f"max_orbit_distance ({max_orbit_distance}) must be >= "
+                f"min_orbit_distance ({self._state.min_orbit_distance})."
+            )
         if np.allclose(self._state.max_orbit_distance, max_orbit_distance):
             return
         self._state.max_orbit_distance = max_orbit_distance
@@ -1059,6 +1087,18 @@ async def _(conn: infra.WebsockClientConnection) -> None:
 
         # Start the server.
         server.start()
+        # The share-tunnel slot must exist BEFORE stop() is registered with
+        # atexit below: stop() reads it, and if __init__ fails later, the
+        # atexit-time stop() on the partially-built server would otherwise
+        # miss the attribute and fall into DeprecatedAttributeShim.__getattr__
+        # (whose `self.scene` lookup recurses without `scene` set -->
+        # RecursionError at interpreter exit).
+        self._share_tunnel: ViserTunnel | None = None
+        # Guards the share-tunnel slot's check-then-act. The handlers below run
+        # on pool threads (not serialized on the event loop), so two
+        # concurrent requests could both see None and each create a tunnel,
+        # orphaning (leaking) the first.
+        self._share_tunnel_lock = threading.Lock()
         # server.start() registered the infra-level WebsockServer.stop with
         # atexit; also register the full ViserServer.stop, which runs first
         # (LIFO) and supersedes it (WebsockServer.stop unregisters itself).
@@ -1135,13 +1175,6 @@ async def _(conn: infra.WebsockClientConnection) -> None:
             )
         )
 
-        self._share_tunnel: ViserTunnel | None = None
-        # Guards the share-tunnel slot's check-then-act. The handlers now run
-        # on pool threads (not serialized on the event loop), so two
-        # concurrent requests could both see None and each create a tunnel,
-        # orphaning (leaking) the first.
-        self._share_tunnel_lock = threading.Lock()
-
         # Create share tunnel if requested.
         # This is deprecated: we should use get_share_url() instead.
         share = _deprecated_kwargs.get("share", False)
@@ -1302,6 +1335,14 @@ def request_share_url(self, verbose: bool = True) -> str | None:
         # OUTSIDE the lock so requests don't serialize on the connection.
         with self._share_tunnel_lock:
             tunnel = self._share_tunnel
+            if tunnel is not None and tunnel.get_status() in ("failed", "closed"):
+                # A tunnel that failed to connect never signals disconnect, so
+                # nothing else clears the slot: without this, every later
+                # request would see the dead tunnel and return None forever.
+                # Close it (its worker already exited, so this is bookkeeping,
+                # not blocking) and let this request create a fresh one.
+                tunnel.close()
+                tunnel = self._share_tunnel = None
             we_created = tunnel is None
             if we_created:
                 if verbose:
@@ -1450,8 +1491,11 @@ def on_client_connect(
         # connect between the two lines.
         for client in clients:
             if asyncio.iscoroutinefunction(cb):
-                task = self._event_loop.create_task(cb(client))
-                task.add_done_callback(print_task_error)
+                # run_coroutine_threadsafe, not create_task: registration
+                # typically happens on a user thread, and create_task is
+                # neither thread-safe nor guaranteed to wake the loop.
+                future = asyncio.run_coroutine_threadsafe(cb(client), self._event_loop)
+                future.add_done_callback(print_task_error)
             else:
                 self._thread_executor.submit(cb, client).add_done_callback(
                     print_threadpool_errors

src/viser/client/src/App.tsx

@@ -414,8 +414,18 @@ function AppLayout({
   canvases: React.ReactNode;
 }) {
   const mantineTheme = useMantineTheme();
-  const useMobileView =
-    useMediaQuery(`(max-width: ${mantineTheme.breakpoints.xs})`) ?? false;
+  // `getInitialValueInEffect: false` makes the first render read matchMedia
+  // synchronously. With the effect-based default, a mobile-width load rendered
+  // desktop-first (`useMobileView === false`), mounted the canvas inside
+  // <ControlPanelDockSurface>, then flipped on the next render -- changing the
+  // element structure around `canvasContent` and remounting the entire canvas
+  // subtree (new WebGL context). This is a client-only app, so there is no SSR
+  // path that needs the deferred read.
+  const useMobileView = useMediaQuery(
+    `(max-width: ${mantineTheme.breakpoints.xs})`,
+    false,
+    { getInitialValueInEffect: false },
+  );
   // The floating layout runs on the docking library: the control panel is a
   // dock panel over the canvas (draggable, dockable to either edge, resizable,
   // minimizable). Sidebar layouts and the mobile bottom sheet are unchanged.

src/viser/client/src/ControlPanel/ControlPanel.tsx

@@ -11,7 +11,7 @@ import { GuiComponentContext } from "./GuiComponentContext";
 import { htmlIconWrapper } from "../components/ComponentStyles.css";
 import { GuiDockContext } from "./GuiDockContext";
 import { DockContext } from "../dock/DockContext";
-import { shallowObjectKeysEqual } from "../utils/shallowObjectKeysEqual";
+import { shallowObjectEqual } from "../utils/shallowObjectKeysEqual";
 import {
   ActionIcon,
   Anchor,
@@ -260,7 +260,11 @@ function MobilePanelSection({ panel }: { panel: GuiPanelMessage }) {
  * honored on this path too); sections sort by the server-side order. */
 function PanelsFallback() {
   const viewer = React.useContext(ViewerContext)!;
-  const panels = viewer.useGui((state) => state.panels, shallowObjectKeysEqual);
+  // Value-level equality, NOT key-set equality: updatePanel replaces the
+  // panel object for the updated uuid (same key set), so a keys-only compare
+  // would swallow visible/order/tab-metadata updates and leave the sheet
+  // rendering stale panel props.
+  const panels = viewer.useGui((state) => state.panels, shallowObjectEqual);
   // On the dock surface, StandalonePanelSync places panels as dock groups -- so
   // this inline fallback must NOT also render them (that would double-render).
   const dockCtx = React.useContext(DockContext);

src/viser/client/src/ControlPanel/panelUpdates.test.ts

@@ -0,0 +1,64 @@
+// Pins the equality function used by PanelsFallback's `state.panels`
+// selector. Regression: shallowObjectKeysEqual compares KEY SETS only, but
+// updatePanel replaces the panel object for the updated uuid without changing
+// the key set -- so prop updates (visible/order/tab metadata) never reached
+// the mobile sheet. The selector must use value-level shallowObjectEqual.
+
+import { describe, expect, it } from "vitest";
+import {
+  shallowObjectEqual,
+  shallowObjectKeysEqual,
+} from "../utils/shallowObjectKeysEqual";
+import { createStore } from "../store";
+
+describe("panels selector equality", () => {
+  it("shallowObjectKeysEqual misses same-key value changes (the old bug)", () => {
+    // Same key set, different value: the keys-only compare reports "equal",
+    // which is exactly why panel prop updates were swallowed.
+    expect(shallowObjectKeysEqual({ a: 1 }, { a: 2 })).toBe(true);
+  });
+
+  it("shallowObjectEqual detects same-key value changes", () => {
+    expect(shallowObjectEqual({ a: 1 }, { a: 2 })).toBe(false);
+    expect(shallowObjectEqual({ a: 1 }, { a: 1 })).toBe(true);
+  });
+
+  it("a subscriber using shallowObjectEqual sees an updatePanel-style change", () => {
+    type Panel = { uuid: string; props: { visible: boolean; order: number } };
+    const panelA: Panel = { uuid: "a", props: { visible: true, order: 0 } };
+    const store = createStore({
+      panels: { a: panelA } as Record<string, Panel>,
+    });
+
+    // Mirror the useStore snapshot logic (selector + equality cache) without
+    // React: on each notification, keep the cached slice iff equalityFn says
+    // it's unchanged.
+    const track = (equalityFn: (a: any, b: any) => boolean) => {
+      let cached = store.get().panels;
+      const seen: Record<string, Panel>[] = [];
+      store.subscribe(() => {
+        const next = store.get().panels;
+        if (!equalityFn(cached, next)) {
+          cached = next;
+          seen.push(next);
+        }
+      });
+      return seen;
+    };
+    const seenByKeys = track(shallowObjectKeysEqual);
+    const seenByValues = track(shallowObjectEqual);
+
+    // updatePanel-style change: replace the panel object for one uuid,
+    // keeping the key set identical (GuiState.updatePanel).
+    store.set((state) => ({
+      panels: {
+        ...state.panels,
+        a: { ...panelA, props: { ...panelA.props, visible: false } },
+      },
+    }));
+
+    expect(seenByKeys).toHaveLength(0); // the old bug: update invisible
+    expect(seenByValues).toHaveLength(1);
+    expect(seenByValues[0].a.props.visible).toBe(false);
+  });
+});