From 0aa4013f3f647a475da50f64816011c44003a273 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Tue, 1 Apr 2025 11:11:10 +0200 Subject: [PATCH] Improve UX when loading JS output fails or takes long time (#2975) --- assets/js/hooks/js_view.js | 52 +++++++++---------- lib/livebook_web/channels/js_view_channel.ex | 32 ++++++++---- lib/livebook_web/live/js_view_component.ex | 7 ++- lib/livebook_web/live/output.ex | 2 +- .../channels/js_view_channel_test.exs | 32 +++++++++--- 5 files changed, 79 insertions(+), 46 deletions(-) diff --git a/assets/js/hooks/js_view.js b/assets/js/hooks/js_view.js index 36fbd7ad0..05777f8b2 100644 --- a/assets/js/hooks/js_view.js +++ b/assets/js/hooks/js_view.js @@ -56,8 +56,8 @@ import { initializeIframeSource } from "./js_view/iframe"; * * * `iframe-url` - an optional location to load the iframe from * - * * `timeout-message` - the message to show when the initial - * data does not load + * * `unreachable-message` - the message to show when the initial + * data fails to load * */ const JSView = { @@ -72,8 +72,6 @@ const JSView = { this.syncCallbackQueue = []; this.pongCallbackQueue = []; - this.initTimeout = setTimeout(() => this.handleInitTimeout(), 2_000); - this.channel = getChannel(this.props.sessionToken); this.iframeActions = this.createIframe(); @@ -102,8 +100,13 @@ const JSView = { const initRef = this.channel.on( `init:${this.props.ref}:${this.id}`, (raw) => { - const [, payload] = transportDecode(raw); - this.handleServerInit(payload); + const [[ok], payload] = transportDecode(raw); + this.removeSkeleton(); + if (ok) { + this.handleServerInit(payload); + } else { + this.handleInitUnreachable(); + } }, ); @@ -115,7 +118,10 @@ const JSView = { const errorRef = this.channel.on( `error:${this.props.ref}`, ({ message, init }) => { - this.handleServerError(message, init); + if (init) { + this.removeSkeleton(); + } + this.handleServerError(message); }, ); @@ -187,7 +193,7 @@ const JSView = { "connect-token", "iframe-port", "iframe-url", - "timeout-message", + "unreachable-message", ]); }, @@ -416,23 +422,14 @@ const JSView = { } }, - handleInitTimeout() { - this.initTimeoutContainer = document.createElement("div"); - this.initTimeoutContainer.classList.add("info-box"); - this.el.prepend(this.initTimeoutContainer); - this.initTimeoutContainer.textContent = this.props.timeoutMessage; - }, - - clearInitTimeout() { - clearTimeout(this.initTimeout); - - if (this.initTimeoutContainer) { - this.initTimeoutContainer.remove(); - } + handleInitUnreachable() { + const container = document.createElement("div"); + container.classList.add("info-box"); + this.el.prepend(container); + container.textContent = this.props.unreachableMessage; }, handleServerInit(payload) { - this.clearInitTimeout(); this.initReceived = true; this.childReadyPromise.then(() => { @@ -450,11 +447,7 @@ const JSView = { }); }, - handleServerError(message, init) { - if (init) { - this.clearInitTimeout(); - } - + handleServerError(message) { if (!this.errorContainer) { this.errorContainer = document.createElement("div"); this.errorContainer.classList.add("error-box", "mb-4"); @@ -497,6 +490,11 @@ const JSView = { parentFocusableId === focusableId, ); }, + + removeSkeleton() { + const skeletonEl = this.el.querySelector(`[data-el-skeleton]`); + skeletonEl.remove(); + }, }; /** diff --git a/lib/livebook_web/channels/js_view_channel.ex b/lib/livebook_web/channels/js_view_channel.ex index 2b11abc91..60692549d 100644 --- a/lib/livebook_web/channels/js_view_channel.ex +++ b/lib/livebook_web/channels/js_view_channel.ex @@ -28,8 +28,12 @@ defmodule LivebookWeb.JSViewChannel do socket = update_in(socket.assigns.ref_with_info[ref], fn - nil -> %{pid: pid, count: 1, connect_queue: [id]} - info -> %{info | count: info.count + 1, connect_queue: info.connect_queue ++ [id]} + nil -> + monitor_ref = Process.monitor(pid, tag: {:connect_down, ref}) + %{pid: pid, monitor_ref: monitor_ref, count: 1, connect_queue: [id]} + + info -> + %{info | count: info.count + 1, connect_queue: info.connect_queue ++ [id]} end) if socket.assigns.ref_with_info[ref].count == 1 do @@ -66,13 +70,10 @@ defmodule LivebookWeb.JSViewChannel do def handle_in("disconnect", %{"ref" => ref}, socket) do socket = case socket.assigns.ref_with_info do - %{^ref => %{count: 1}} -> - Livebook.Session.unsubscribe_from_runtime_events( - socket.assigns.session_id, - "js_live", - ref - ) - + %{^ref => %{count: 1, monitor_ref: monitor_ref}} -> + Process.demonitor(monitor_ref, [:flush]) + session_id = socket.assigns.session_id + Livebook.Session.unsubscribe_from_runtime_events(session_id, "js_live", ref) {_, socket} = pop_in(socket.assigns.ref_with_info[ref]) socket @@ -97,7 +98,7 @@ defmodule LivebookWeb.JSViewChannel do {id, queue} end) - with {:error, error} <- try_push(socket, "init:#{ref}:#{id}", nil, payload) do + with {:error, error} <- try_push(socket, "init:#{ref}:#{id}", [true], payload) do message = "Failed to serialize initial widget data, " <> error push(socket, "error:#{ref}", %{"message" => message, "init" => true}) end @@ -105,6 +106,17 @@ defmodule LivebookWeb.JSViewChannel do {:noreply, socket} end + def handle_info({{:connect_down, ref}, _ref, :process, _pid, _reason}, socket) do + Livebook.Session.unsubscribe_from_runtime_events(socket.assigns.session_id, "js_live", ref) + {%{connect_queue: ids}, socket} = pop_in(socket.assigns.ref_with_info[ref]) + + for id <- ids do + :ok = try_push(socket, "init:#{ref}:#{id}", [false], nil) + end + + {:noreply, socket} + end + def handle_info({:event, event, payload, %{ref: ref}}, socket) do with {:error, error} <- try_push(socket, "event:#{ref}", [event], payload) do message = "Failed to serialize widget data, " <> error diff --git a/lib/livebook_web/live/js_view_component.ex b/lib/livebook_web/live/js_view_component.ex index 9d8c7f692..41ff57cf1 100644 --- a/lib/livebook_web/live/js_view_component.ex +++ b/lib/livebook_web/live/js_view_component.ex @@ -6,7 +6,7 @@ defmodule LivebookWeb.JSViewComponent do {:ok, socket |> assign(assigns) - |> assign_new(:timeout_message, fn -> "Not available" end)} + |> assign_new(:unreachable_message, fn -> "Not available" end)} end @impl true @@ -26,8 +26,11 @@ defmodule LivebookWeb.JSViewComponent do data-p-connect-token={hook_prop(connect_token(@js_view.pid))} data-p-iframe-port={hook_prop(LivebookWeb.IframeEndpoint.port())} data-p-iframe-url={hook_prop(Livebook.Config.iframe_url())} - data-p-timeout-message={hook_prop(@timeout_message)} + data-p-unreachable-message={hook_prop(@unreachable_message)} > +
+ <.content_skeleton empty={false} /> +
""" end diff --git a/lib/livebook_web/live/output.ex b/lib/livebook_web/live/output.ex index b7965c67d..69dec2431 100644 --- a/lib/livebook_web/live/output.ex +++ b/lib/livebook_web/live/output.ex @@ -91,7 +91,7 @@ defmodule LivebookWeb.Output do js_view={@js_view} session_id={@session_id} client_id={@client_id} - timeout_message="Output data no longer available, please reevaluate this cell" + unreachable_message="Output data no longer available, please reevaluate this cell" /> """ end diff --git a/test/livebook_web/channels/js_view_channel_test.exs b/test/livebook_web/channels/js_view_channel_test.exs index 26fee5158..cee370c7c 100644 --- a/test/livebook_web/channels/js_view_channel_test.exs +++ b/test/livebook_web/channels/js_view_channel_test.exs @@ -20,7 +20,7 @@ defmodule LivebookWeb.JSViewChannelTest do assert_receive {:connect, from, %{}} send(from, {:connect_reply, [1, 2, 3], %{ref: "1"}}) - assert_push "init:1:id1", %{"root" => [nil, [1, 2, 3]]} + assert_push "init:1:id1", %{"root" => [[true], [1, 2, 3]]} end test "loads initial data for multiple connections separately", %{socket: socket} do @@ -29,11 +29,31 @@ defmodule LivebookWeb.JSViewChannelTest do assert_receive {:connect, from, %{}} send(from, {:connect_reply, [1, 2, 3], %{ref: "1"}}) - assert_push "init:1:id1", %{"root" => [nil, [1, 2, 3]]} + assert_push "init:1:id1", %{"root" => [[true], [1, 2, 3]]} assert_receive {:connect, from, %{}} send(from, {:connect_reply, [1, 2, 3], %{ref: "1"}}) - assert_push "init:1:id2", %{"root" => [nil, [1, 2, 3]]} + assert_push "init:1:id2", %{"root" => [[true], [1, 2, 3]]} + end + + test "sends init failure when the widget server terminates", %{socket: socket} do + widget_server_pid = + spawn(fn -> + # Respond only to the first one and terminate + receive do + {:connect, from, %{}} -> + send(from, {:connect_reply, [1, 2, 3], %{ref: "1"}}) + end + end) + + connect_token = connect_token(widget_server_pid) + + push(socket, "connect", %{"connect_token" => connect_token, "ref" => "1", "id" => "id1"}) + push(socket, "connect", %{"connect_token" => connect_token, "ref" => "1", "id" => "id2"}) + + assert_push "init:1:id1", %{"root" => [[true], [1, 2, 3]]} + + assert_push "init:1:id2", %{"root" => [[false], nil]} end test "sends client events to the corresponding widget server", %{socket: socket} do @@ -74,7 +94,7 @@ defmodule LivebookWeb.JSViewChannelTest do send(from, {:connect_reply, payload, %{ref: "1"}}) assert_push "init:1:id1", - {:binary, <<24::size(32), "[null,{\"message\":\"hey\"}]", 1, 2, 3>>} + {:binary, <<26::size(32), "[[true],{\"message\":\"hey\"}]", 1, 2, 3>>} end test "form client to server", %{socket: socket} do @@ -98,7 +118,7 @@ defmodule LivebookWeb.JSViewChannelTest do }) end - defp connect_token() do - Phoenix.Token.sign(LivebookWeb.Endpoint, "js-view-connect", %{pid: self()}) + defp connect_token(pid \\ self()) do + Phoenix.Token.sign(LivebookWeb.Endpoint, "js-view-connect", %{pid: pid}) end end