fix: show users an error notification when a widget fails after render (#320)

CHANGELOG.md

@@ -90,6 +90,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Bug fixes
 
+* A widget that fails after its render function returned no longer leaves the output silently blank (or, on older Shiny, ends the session). Failures while a widget's comm opens (e.g., an error in a display hook or in state serialization) and failures in widget callbacks triggered by browser messages (e.g., an Altair/VegaFusion chart without `vl-convert-python` installed) now show the user a sticky error notification, deduplicated per widget. The full error message is shown locally; where the app sanitizes errors (e.g., Posit Connect), the notification uses the generic sanitized message. The error continues to be logged server-side. (#320)
 * Fixed many issues with widgets nested inside other widgets and with widget outputs inserted dynamically (e.g., `output_widget()` inside `@render.ui`). With ipywidgets 8, these either didn't render at all (e.g., a Plotly `FigureWidget` or a styled `HTML` widget inside an ipyleaflet popup) or rendered while logging errors like `Class HTMLStyleModel not found` (e.g., ipyleaflet popups, `WidgetControl` children, and leafmap legends). (#75, #101, #137, #157, #194, #237)
 * Re-rendering, replacing, hiding/showing, or removing widget outputs no longer leaks memory on the server or in the browser. Re-rendered widgets, and the data they hold, are now freed right away instead of when the session ends. (#246, #258, #260, #261, #262, #263, #303, #405) Related fixes:
   * pythreejs widgets no longer cause Shiny sessions to hang indefinitely with ipywidgets 8. (#244)

shinywidgets/_comm.py

@@ -6,7 +6,7 @@
 from dataclasses import dataclass, field
 from typing import TYPE_CHECKING, Callable, Dict, List, Optional
 
-from shiny import Session
+from shiny import Session, ui
 
 if TYPE_CHECKING:
     from ipywidgets import Widget
@@ -258,6 +258,8 @@ def dispatch_client_msg(
             traitlets_level_patch.restore()
             scope.active = False
             handling_client_msg.reset(token)
+        if scope.errors:
+            notify_client_msg_error(self, scope.errors[0])
 
     def handle_close(self, msg: Dict[str, object]) -> None:
         """The browser closed this comm: a later close() has nothing to publish."""
@@ -513,6 +515,13 @@ class ClientMsgScope:
 
     active = True
 
+    def __init__(self) -> None:
+        # The callback errors rerouted during dispatch, for its user-facing
+        # notification (see notify_client_msg_error). Their messages, not the
+        # exceptions: deferred work can outlive the dispatch holding this
+        # scope, and a traceback's frames would pin the widget and session.
+        self.errors: List[str] = []
+
 
 handling_client_msg: ContextVar[Optional[ClientMsgScope]] = ContextVar(
     "shinywidgets_handling_client_msg", default=None
@@ -562,6 +571,27 @@ def restore(self) -> None:
 traitlets_level_patch = TraitletsLevelPatch()
 
 
+def notify_client_msg_error(comm: ShinyComm, msg: str) -> None:
+    """
+    Show a sticky browser notification for a widget-callback error that
+    ipywidgets swallowed (see ClientMsgErrorFilter): the error already
+    reached the server log, but if the failing callback is part of how the
+    widget renders, the user's output is otherwise blank with no
+    explanation. The browser replaces a notification whose id it already
+    shows, so a callback that fails on every message leaves one per widget.
+    """
+    session = comm.session
+    if session.app.sanitize_errors:
+        msg = session.app.sanitize_error_msg
+    ui.notification_show(
+        msg,
+        type="error",
+        duration=None,
+        id=f"shinywidgets-error-{comm.comm_id}",
+        session=session,
+    )
+
+
 class ClientMsgErrorFilter(logging.Filter):
     """
     Outside IPython, ipywidgets catches exceptions raised by widget callbacks
@@ -571,13 +601,19 @@ class ClientMsgErrorFilter(logging.Filter):
     While handling a browser message, move such records to the `shinywidgets`
     logger (dropping the original, so a configured root handler doesn't show
     them twice). Everywhere else, e.g. in Jupyter, `traitlets` logging is untouched.
+
+    A rerouted exception is also noted on the dispatch's scope, so the
+    dispatch can show it to the user (see notify_client_msg_error).
     """
 
     def filter(self, record: logging.LogRecord) -> bool:
         scope = handling_client_msg.get()
         if scope is None or not scope.active or record.levelno < logging.WARNING:
             return True
         logger.error(record.getMessage(), exc_info=record.exc_info)
+        exc = record.exc_info[1] if record.exc_info else None
+        if exc is not None:
+            scope.errors.append(str(exc))
         return False
 
 

shinywidgets/_shinywidgets.py

@@ -43,6 +43,7 @@
     get_current_context,  # pyright: ignore[reportPrivateImportUsage]
 )
 from shiny.session import get_current_session, require_active_session, session_context
+from shiny.types import NotifyException
 
 from ._accessors import rate_policy_for
 from ._as_widget import as_widget, pydeck_has_widget
@@ -250,59 +251,83 @@ def _open_shiny_comm():
         if w is None or WIDGET_INSTANCE_MAP.get(id) is not w:
             return
 
-        if is_ipyreact_registry_widget(w):
-            fan_out_ipyreact_registry(w)
-            return
+        try:
+            if is_ipyreact_registry_widget(w):
+                fan_out_ipyreact_registry(w)
+                return
 
-        # Already opened in another session by open_shared_in(); a plain
-        # ShinyComm here would cut that session off.
-        if isinstance(w.comm, SharedShinyComm):
-            open_shared_in(w, session)
-            return
+            # Already opened in another session by open_shared_in(); a plain
+            # ShinyComm here would cut that session off.
+            if isinstance(w.comm, SharedShinyComm):
+                open_shared_in(w, session)
+                return
+
+            # A shared widget's lazily created defaults (e.g. its Layout) are shared too
+            with (
+                shared_construction()
+                if shared
+                else WidgetRenderContext.resumed(render_ctx)
+            ):
+                prepare_to_serialize(w)
+                # Serve geolibre's bundled app from the Shiny app and switch the
+                # widget to the mode that probes that route. This must happen here
+                # (not in init_shiny_widget()): that hook runs inside
+                # Widget.__init__(), and geolibre's Map.__init__() sets
+                # _remote_mode after super().__init__() returns, overwriting it.
+                if any(widget_pkg(cls) == "geolibre" for cls in type(w).__mro__):
+                    geolibre_app_mount(w, session)
+                # Mount a guarded loopback proxy for localtileserver-backed
+                # ipyleaflet layers and route their tile URLs through it (no-op
+                # for other widgets). Must run before get_state() so the browser
+                # only ever sees the rewritten (same-origin) URL.
+                localtileserver_layer_proxy(w, session)
+                state, buffer_paths, buffers = _remove_buffers(w.get_state())
+
+            # Covers widgets that are opened but never displayed (so no render-time
+            # walk opens their references); the browser would otherwise wait for a
+            # model it never gets. Foreign references are left for the render to reject.
+            for r in direct_widget_references(w):
+                if is_process_global(r):
+                    open_shared_in(r, session)
 
-        # A shared widget's lazily created defaults (e.g. its Layout) are shared too
-        with (
-            shared_construction() if shared else WidgetRenderContext.resumed(render_ctx)
-        ):
-            prepare_to_serialize(w)
-            # Serve geolibre's bundled app from the Shiny app and switch the
-            # widget to the mode that probes that route. This must happen here
-            # (not in init_shiny_widget()): that hook runs inside
-            # Widget.__init__(), and geolibre's Map.__init__() sets
-            # _remote_mode after super().__init__() returns, overwriting it.
-            if any(widget_pkg(cls) == "geolibre" for cls in type(w).__mro__):
-                geolibre_app_mount(w, session)
-            # Mount a guarded loopback proxy for localtileserver-backed
-            # ipyleaflet layers and route their tile URLs through it (no-op
-            # for other widgets). Must run before get_state() so the browser
-            # only ever sees the rewritten (same-origin) URL.
-            localtileserver_layer_proxy(w, session)
-            state, buffer_paths, buffers = _remove_buffers(w.get_state())
-
-        # Covers widgets that are opened but never displayed (so no render-time
-        # walk opens their references); the browser would otherwise wait for a
-        # model it never gets. Foreign references are left for the render to reject.
-        for r in direct_widget_references(w):
-            if is_process_global(r):
-                open_shared_in(r, session)
-
-        # Initialize the comm -- this sends widget state to the frontend
-        placeholder = w.comm
-        w.comm = ShinyComm(
-            comm_id=id,
-            comm_manager=COMM_MANAGERS[session.id],
-            session=session,
-            target_name="jupyter.widget",
-            data={"state": state, "buffer_paths": buffer_paths},
-            buffers=cast(BufferType, buffers),
-            # TODO: should this be hard-coded?
-            metadata={"version": __protocol_version__},
-            html_deps=session._process_ui(TagList(*widget_deps))["deps"],
-            **open_keys,
-        )
+            # Initialize the comm -- this sends widget state to the frontend
+            placeholder = w.comm
+            w.comm = ShinyComm(
+                comm_id=id,
+                comm_manager=COMM_MANAGERS[session.id],
+                session=session,
+                target_name="jupyter.widget",
+                data={"state": state, "buffer_paths": buffer_paths},
+                buffers=cast(BufferType, buffers),
+                # TODO: should this be hard-coded?
+                metadata={"version": __protocol_version__},
+                html_deps=session._process_ui(TagList(*widget_deps))["deps"],
+                **open_keys,
+            )
 
-        flush_ipycanvas_manager(w)
-        flush_queued_sends(placeholder, w.comm)
+            flush_ipycanvas_manager(w)
+            flush_queued_sends(placeholder, w.comm)
+        except Exception as e:
+            # The render already returned, so Shiny's render-error display
+            # never sees this failure; unhandled, the output stays blank
+            # forever (and on older Shiny the error would end the session).
+            # NotifyException makes Shiny print the traceback and show a
+            # sticky error notification while the session survives -- it
+            # fires even though this effect destroyed itself above.
+            #
+            # A process-global widget belongs to every session, so one
+            # session's failure must not close it. A session-owned one is
+            # closed, tearing down its half-initialized state the same way
+            # render invalidation would.
+            if not is_process_global(w):
+                try:
+                    close_widget(w)
+                except Exception:
+                    logger.exception("Failed to close widget after comm-open failure")
+            raise NotifyException(
+                f"Failed to display {type(w).__name__}: {e}",
+                sanitize=session.app.sanitize_errors,
+            ) from e
 
     # If the widget initialized in a reactive _output_ context, then cleanup the widget
     # when the context gets invalidated. Use the render output's Context (captured by

tests/unit/test_callback_errors.py

@@ -2,7 +2,9 @@
 Exceptions raised by widget callbacks while handling a browser message are
 caught by ipywidgets and logged to the `traitlets` logger, whose only handler is
 a NullHandler, so outside of IPython they would vanish. shinywidgets re-routes
-them to its own logger so they show up in the server's console.
+them to its own logger so they show up in the server's console, and shows the
+user a sticky error notification (one per widget), since the error usually
+means their output is blank.
 """
 
 from __future__ import annotations
@@ -16,7 +18,7 @@
 from ipywidgets import Button, IntSlider
 from shiny import Session, reactive
 from shiny.session import session_context
-from shinywidgets._comm import ShinyComm
+from shinywidgets._comm import ClientMsgScope, ShinyComm, handling_client_msg
 
 
 def flush() -> None:
@@ -208,3 +210,144 @@ def schedule(change: object) -> None:
     [record] = caplog.records
     assert record.name == "traitlets"
     assert record.levelno == logging.WARNING
+
+
+def test_scope_keeps_error_messages_not_exceptions(widget_session: Session) -> None:
+    # Deferred work (asyncio tasks, loop.call_soon) shares the dispatch scope
+    # via its copied context, so an exception kept on it would keep its
+    # traceback's frames -- and through them the widget, its callbacks and
+    # session -- alive for the lifetime of that work, even after the widget
+    # is closed or replaced.
+    scopes: List[ClientMsgScope] = []
+
+    def boom(change: object) -> None:
+        scope = handling_client_msg.get()
+        assert scope is not None
+        scopes.append(scope)
+        raise RuntimeError("observer failed")
+
+    with session_context(widget_session):
+        slider = IntSlider(value=1)
+        slider.observe(boom, names="value")
+        comm = open_comm(slider)
+        comm.handle_msg(
+            client_msg(
+                comm.comm_id,
+                {"method": "update", "state": {"value": 5}, "buffer_paths": []},
+            )
+        )
+
+    [scope] = scopes
+    assert scope.errors == ["observer failed"]
+
+
+def capture_notifications(
+    session: Session, monkeypatch: pytest.MonkeyPatch
+) -> List[Dict[str, Any]]:
+    sent: List[Dict[str, Any]] = []
+    monkeypatch.setattr(session, "_send_message_sync", sent.append)
+    return sent
+
+
+def failing_slider() -> ShinyComm:
+    def boom(change: object) -> None:
+        raise RuntimeError("observer failed")
+
+    slider = IntSlider(value=1)
+    slider.observe(boom, names="value")
+    comm = open_comm(slider)
+    comm.handle_msg(
+        client_msg(
+            comm.comm_id,
+            {"method": "update", "state": {"value": 5}, "buffer_paths": []},
+        )
+    )
+    return comm
+
+
+def test_observer_error_notifies_browser(
+    widget_session: Session,
+    monkeypatch: pytest.MonkeyPatch,
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    sent = capture_notifications(widget_session, monkeypatch)
+    with session_context(widget_session), caplog.at_level(logging.WARNING):
+        failing_slider()
+
+    # Still logged server-side (the #304 behavior) ...
+    [record] = caplog.records
+    assert record.name == "shinywidgets"
+    # ... and now also shown to the user as a sticky error notification.
+    [note] = [m for m in sent if "notification" in m]
+    payload = note["notification"]["message"]
+    assert payload["type"] == "error"
+    assert payload["duration"] is None
+    assert "observer failed" in payload["html"]
+
+
+def test_repeat_errors_reuse_the_widgets_notification(
+    widget_session: Session,
+    monkeypatch: pytest.MonkeyPatch,
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    sent = capture_notifications(widget_session, monkeypatch)
+    with session_context(widget_session), caplog.at_level(logging.WARNING):
+        comm = failing_slider()
+        comm.handle_msg(
+            client_msg(
+                comm.comm_id,
+                {"method": "update", "state": {"value": 6}, "buffer_paths": []},
+            )
+        )
+        other = failing_slider()
+
+    # Every failure is logged ...
+    assert len(caplog.records) == 3
+    # ... but the browser replaces a notification whose id it already shows,
+    # so an observer that fails on every message leaves one per widget.
+    ids = [m["notification"]["message"]["id"] for m in sent if "notification" in m]
+    assert ids == [
+        f"shinywidgets-error-{comm.comm_id}",
+        f"shinywidgets-error-{comm.comm_id}",
+        f"shinywidgets-error-{other.comm_id}",
+    ]
+
+
+def test_warning_without_exception_does_not_notify(
+    widget_session: Session,
+    monkeypatch: pytest.MonkeyPatch,
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    def warn(change: object) -> None:
+        logging.getLogger("traitlets").warning("heads up")
+
+    sent = capture_notifications(widget_session, monkeypatch)
+    with session_context(widget_session), caplog.at_level(logging.WARNING):
+        slider = IntSlider(value=1)
+        slider.observe(warn, names="value")
+        comm = open_comm(slider)
+        comm.handle_msg(
+            client_msg(
+                comm.comm_id,
+                {"method": "update", "state": {"value": 5}, "buffer_paths": []},
+            )
+        )
+
+    assert "heads up" in caplog.text
+    assert [m for m in sent if "notification" in m] == []
+
+
+def test_observer_error_notification_sanitized_when_app_sanitizes(
+    widget_session: Session,
+    monkeypatch: pytest.MonkeyPatch,
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    widget_session.app.sanitize_errors = True
+    sent = capture_notifications(widget_session, monkeypatch)
+    with session_context(widget_session), caplog.at_level(logging.WARNING):
+        failing_slider()
+
+    [note] = [m for m in sent if "notification" in m]
+    assert note["notification"]["message"]["html"] == (
+        "An error has occurred. Check your logs or contact the app author for clarification."
+    )