From bd8e06b5ce063e5876f234acedc3d0a616ef94d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Thu, 8 Jul 2021 19:35:11 +0200 Subject: [PATCH] Implement relative navigation between notebooks (#441) * Use live redirect for local links in rendered markdown * Resolve relative notebook URLs * Bump LV * Adds tests * Handle nested relative path * Handle child nested paths --- assets/js/cell/markdown.js | 30 +++-- lib/livebook_web/live/home_live.ex | 16 --- lib/livebook_web/live/session_helpers.ex | 24 ++++ lib/livebook_web/live/session_live.ex | 91 ++++++++++++-- lib/livebook_web/router.ex | 3 + mix.lock | 2 +- test/livebook_web/live/session_live_test.exs | 123 +++++++++++++++++++ 7 files changed, 253 insertions(+), 36 deletions(-) diff --git a/assets/js/cell/markdown.js b/assets/js/cell/markdown.js index 40351795f..592a3b65f 100644 --- a/assets/js/cell/markdown.js +++ b/assets/js/cell/markdown.js @@ -4,8 +4,22 @@ import DOMPurify from "dompurify"; import katex from "katex"; import { highlight } from "./live_editor/monaco"; -// Reuse Monaco highlighter for Markdown code blocks +// Custom renderer overrides +const renderer = new marked.Renderer(); +renderer.link = function (href, title, text) { + // Browser normalizes URLs with .. so we use a __parent__ modifier + // instead and handle it on the server + href = href + .split("/") + .map((part) => (part === ".." ? "__parent__" : part)) + .join("/"); + + return marked.Renderer.prototype.link.call(this, href, title, text); +}; + marked.setOptions({ + renderer, + // Reuse Monaco highlighter for Markdown code blocks highlight: (code, lang, callback) => { highlight(code, lang) .then((html) => callback(null, html)) @@ -16,12 +30,14 @@ marked.setOptions({ // Modify external links, so that they open in a new tab. // See https://github.com/cure53/DOMPurify/tree/main/demos#hook-to-open-all-links-in-a-new-window-link DOMPurify.addHook("afterSanitizeAttributes", (node) => { - if ( - node.tagName.toLowerCase() === "a" && - node.host !== window.location.host - ) { - node.setAttribute("target", "_blank"); - node.setAttribute("rel", "noreferrer noopener"); + if (node.tagName.toLowerCase() === "a") { + if (node.host !== window.location.host) { + node.setAttribute("target", "_blank"); + node.setAttribute("rel", "noreferrer noopener"); + } else { + node.setAttribute("data-phx-link", "redirect"); + node.setAttribute("data-phx-link-state", "push"); + } } }); diff --git a/lib/livebook_web/live/home_live.ex b/lib/livebook_web/live/home_live.ex index c679acf26..3421be622 100644 --- a/lib/livebook_web/live/home_live.ex +++ b/lib/livebook_web/live/home_live.ex @@ -311,22 +311,6 @@ defmodule LivebookWeb.HomeLive do LiveMarkdown.Import.notebook_from_markdown(content) end - defp put_import_flash_messages(socket, []), do: socket - - defp put_import_flash_messages(socket, messages) do - list = - messages - |> Enum.map(fn message -> ["- ", message] end) - |> Enum.intersperse("\n") - - flash = - IO.iodata_to_binary([ - "We found problems while importing the file and tried to autofix them:\n" | list - ]) - - put_flash(socket, :info, flash) - end - defp session_id_by_path(path, session_summaries) do summary = Enum.find(session_summaries, &(&1.path == path)) summary.session_id diff --git a/lib/livebook_web/live/session_helpers.ex b/lib/livebook_web/live/session_helpers.ex index 4a16b8d3c..6a2e94827 100644 --- a/lib/livebook_web/live/session_helpers.ex +++ b/lib/livebook_web/live/session_helpers.ex @@ -18,4 +18,28 @@ defmodule LivebookWeb.SessionHelpers do put_flash(socket, :error, "Failed to create session: #{reason}") end end + + @doc """ + Formats the given list of notebook import messages and puts + into the info flash. + """ + @spec put_import_flash_messages(Phoenix.LiveView.Socket.t(), list(String.t())) :: + Phoenix.LiveView.Socket.t() + def put_import_flash_messages(socket, messages) + + def put_import_flash_messages(socket, []), do: socket + + def put_import_flash_messages(socket, messages) do + list = + messages + |> Enum.map(fn message -> ["- ", message] end) + |> Enum.intersperse("\n") + + flash = + IO.iodata_to_binary([ + "We found problems while importing the file and tried to autofix them:\n" | list + ]) + + put_flash(socket, :info, flash) + end end diff --git a/lib/livebook_web/live/session_live.ex b/lib/livebook_web/live/session_live.ex index 070bdec24..ec1493dee 100644 --- a/lib/livebook_web/live/session_live.ex +++ b/lib/livebook_web/live/session_live.ex @@ -2,10 +2,11 @@ defmodule LivebookWeb.SessionLive do use LivebookWeb, :live_view import LivebookWeb.UserHelpers + import LivebookWeb.SessionHelpers import Livebook.Utils, only: [access_by_id: 1] alias LivebookWeb.SidebarHelpers - alias Livebook.{SessionSupervisor, Session, Delta, Notebook, Runtime} + alias Livebook.{SessionSupervisor, Session, Delta, Notebook, Runtime, LiveMarkdown} alias Livebook.Notebook.Cell @impl true @@ -326,6 +327,21 @@ defmodule LivebookWeb.SessionLive do {:noreply, assign(socket, section: section, first_section_id: first_section_id)} end + def handle_params( + %{"path_parts" => path_parts}, + _url, + %{assigns: %{live_action: :catch_all}} = socket + ) do + path_parts = + Enum.map(path_parts, fn + "__parent__" -> ".." + part -> part + end) + + path = Path.join(path_parts) + {:noreply, handle_relative_path(socket, path)} + end + def handle_params(_params, _url, socket) do {:noreply, socket} end @@ -608,7 +624,7 @@ defmodule LivebookWeb.SessionLive do data = Session.get_data(socket.assigns.session_id) notebook = Notebook.forked(data.notebook) %{images_dir: images_dir} = Session.get_summary(socket.assigns.session_id) - create_session(socket, notebook: notebook, copy_images_from: images_dir) + {:noreply, create_session(socket, notebook: notebook, copy_images_from: images_dir)} end def handle_event("location_report", report, socket) do @@ -635,16 +651,6 @@ defmodule LivebookWeb.SessionLive do {:reply, %{code: formatted}, socket} end - defp create_session(socket, opts) do - case SessionSupervisor.create_session(opts) do - {:ok, id} -> - {:noreply, push_redirect(socket, to: Routes.session_path(socket, :page, id))} - - {:error, reason} -> - {:noreply, put_flash(socket, :error, "Failed to create a notebook: #{reason}")} - end - end - @impl true def handle_info({:operation, operation}, socket) do case Session.Data.apply_operation(socket.private.data, operation) do @@ -720,6 +726,67 @@ defmodule LivebookWeb.SessionLive do def handle_info(_message, socket), do: {:noreply, socket} + defp handle_relative_path(socket, path) do + cond do + String.ends_with?(path, LiveMarkdown.extension()) -> + handle_relative_notebook_path(socket, path) + + true -> + socket + |> push_patch(to: Routes.session_path(socket, :page, socket.assigns.session_id)) + |> put_flash( + :error, + "Got unrecognised session path: #{path}\nIf you want to link another notebook, make sure to include the .livemd extension" + ) + end + end + + defp handle_relative_notebook_path(socket, relative_path) do + case socket.private.data.path do + nil -> + socket + |> put_flash( + :info, + "Cannot resolve notebook path #{relative_path}, because the current notebook has no file" + ) + |> push_patch(to: Routes.session_path(socket, :page, socket.assigns.session_id)) + + path -> + target_path = path |> Path.dirname() |> Path.join(relative_path) |> Path.expand() + maybe_open_notebook(socket, target_path) + end + end + + defp maybe_open_notebook(socket, path) do + if session_id = session_id_by_path(path) do + push_redirect(socket, to: Routes.session_path(socket, :page, session_id)) + else + case File.read(path) do + {:ok, content} -> + {notebook, messages} = LiveMarkdown.Import.notebook_from_markdown(content) + + socket + |> put_import_flash_messages(messages) + |> create_session(notebook: notebook, path: path) + + {:error, error} -> + message = :file.format_error(error) + + socket + |> put_flash(:error, "Failed to open #{path}, reason: #{message}") + |> push_patch(to: Routes.session_path(socket, :page, socket.assigns.session_id)) + end + end + end + + defp session_id_by_path(path) do + session_summaries = SessionSupervisor.get_session_summaries() + + Enum.find_value(session_summaries, fn summary -> + summary.path == path && summary.session_id + end) + end + defp after_operation(socket, _prev_socket, {:client_join, client_pid, user}) do push_event(socket, "client_joined", %{client: client_info(client_pid, user)}) end diff --git a/lib/livebook_web/router.ex b/lib/livebook_web/router.ex index 95bacabe0..efe92a58e 100644 --- a/lib/livebook_web/router.ex +++ b/lib/livebook_web/router.ex @@ -23,9 +23,11 @@ defmodule LivebookWeb.Router do live "/home/user-profile", HomeLive, :user live "/home/import/:tab", HomeLive, :import live "/home/sessions/:session_id/close", HomeLive, :close_session + live "/explore", ExploreLive, :page live "/explore/user-profile", ExploreLive, :user live "/explore/notebooks/:slug", ExploreLive, :notebook + live "/sessions/:id", SessionLive, :page live "/sessions/:id/user-profile", SessionLive, :user live "/sessions/:id/shortcuts", SessionLive, :shortcuts @@ -36,6 +38,7 @@ defmodule LivebookWeb.Router do live "/sessions/:id/cell-upload/:cell_id", SessionLive, :cell_upload live "/sessions/:id/delete-section/:section_id", SessionLive, :delete_section get "/sessions/:id/images/:image", SessionController, :show_image + live "/sessions/:id/*path_parts", SessionLive, :catch_all live_dashboard "/dashboard", metrics: LivebookWeb.Telemetry, diff --git a/mix.lock b/mix.lock index 208e98f42..6a3f3baeb 100644 --- a/mix.lock +++ b/mix.lock @@ -14,7 +14,7 @@ "phoenix_html": {:git, "https://github.com/phoenixframework/phoenix_html.git", "d35bebbea395569573ef0e1757cbec735da0573b", []}, "phoenix_live_dashboard": {:git, "https://github.com/phoenixframework/phoenix_live_dashboard.git", "1cc67e3c7275b8e68d8201e5dc3660893ae9e4ec", []}, "phoenix_live_reload": {:hex, :phoenix_live_reload, "1.3.3", "3a53772a6118d5679bf50fc1670505a290e32a1d195df9e069d8c53ab040c054", [:mix], [{:file_system, "~> 0.2.1 or ~> 0.3", [hex: :file_system, repo: "hexpm", optional: false]}, {:phoenix, "~> 1.4", [hex: :phoenix, repo: "hexpm", optional: false]}], "hexpm", "766796676e5f558dbae5d1bdb066849673e956005e3730dfd5affd7a6da4abac"}, - "phoenix_live_view": {:git, "https://github.com/phoenixframework/phoenix_live_view.git", "92100b658b257a9dffc11a4ca13e4e9054048f61", []}, + "phoenix_live_view": {:git, "https://github.com/phoenixframework/phoenix_live_view.git", "dcdde14ba2a42908bc7b55d1662e9e33c667ed0e", []}, "phoenix_pubsub": {:hex, :phoenix_pubsub, "2.0.0", "a1ae76717bb168cdeb10ec9d92d1480fec99e3080f011402c0a2d68d47395ffb", [:mix], [], "hexpm", "c52d948c4f261577b9c6fa804be91884b381a7f8f18450c5045975435350f771"}, "phoenix_view": {:git, "https://github.com/phoenixframework/phoenix_view.git", "90ce9c9ef5f832f80e956b77d079f79171ed45d0", []}, "plug": {:hex, :plug, "1.11.1", "f2992bac66fdae679453c9e86134a4201f6f43a687d8ff1cd1b2862d53c80259", [:mix], [{:mime, "~> 1.0", [hex: :mime, repo: "hexpm", optional: false]}, {:plug_crypto, "~> 1.1.1 or ~> 1.2", [hex: :plug_crypto, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "23524e4fefbb587c11f0833b3910bfb414bf2e2534d61928e920f54e3a1b881f"}, diff --git a/test/livebook_web/live/session_live_test.exs b/test/livebook_web/live/session_live_test.exs index fe33b317d..c84962d42 100644 --- a/test/livebook_web/live/session_live_test.exs +++ b/test/livebook_web/live/session_live_test.exs @@ -405,6 +405,129 @@ defmodule LivebookWeb.SessionLiveTest do end end + describe "relative paths" do + test "renders an info message when the path doesn't have notebook extension", + %{conn: conn, session_id: session_id} do + session_path = "/sessions/#{session_id}" + + assert {:error, {:live_redirect, %{to: ^session_path}}} = + result = live(conn, "/sessions/#{session_id}/document.pdf") + + {:ok, view, _} = follow_redirect(result, conn) + assert render(view) =~ "Got unrecognised session path: document.pdf" + end + + test "renders an info message when the session has no associated path", + %{conn: conn, session_id: session_id} do + session_path = "/sessions/#{session_id}" + + assert {:error, {:live_redirect, %{to: ^session_path}}} = + result = live(conn, "/sessions/#{session_id}/notebook.livemd") + + {:ok, view, _} = follow_redirect(result, conn) + + assert render(view) =~ + "Cannot resolve notebook path notebook.livemd, because the current notebook has no file" + end + + @tag :tmp_dir + test "renders an error message when the relative notebook does not exist", + %{conn: conn, session_id: session_id, tmp_dir: tmp_dir} do + index_path = Path.join(tmp_dir, "index.livemd") + notebook_path = Path.join(tmp_dir, "notebook.livemd") + + Session.set_path(session_id, index_path) + wait_for_session_update(session_id) + + session_path = "/sessions/#{session_id}" + + assert {:error, {:live_redirect, %{to: ^session_path}}} = + result = live(conn, "/sessions/#{session_id}/notebook.livemd") + + {:ok, view, _} = follow_redirect(result, conn) + + assert render(view) =~ + "Failed to open #{notebook_path}, reason: no such file or directory" + end + + @tag :tmp_dir + test "opens a relative notebook if it exists", + %{conn: conn, session_id: session_id, tmp_dir: tmp_dir} do + index_path = Path.join(tmp_dir, "index.livemd") + notebook_path = Path.join(tmp_dir, "notebook.livemd") + + Session.set_path(session_id, index_path) + wait_for_session_update(session_id) + + File.write!(notebook_path, "# Sibling notebook") + + assert {:error, {:live_redirect, %{to: _session_path}}} = + result = live(conn, "/sessions/#{session_id}/notebook.livemd") + + {:ok, view, _} = follow_redirect(result, conn) + + assert render(view) =~ "Sibling notebook" + end + + @tag :tmp_dir + test "if the notebook is already open, redirects to the session", + %{conn: conn, session_id: session_id, tmp_dir: tmp_dir} do + index_path = Path.join(tmp_dir, "index.livemd") + notebook_path = Path.join(tmp_dir, "notebook.livemd") + + Session.set_path(session_id, index_path) + wait_for_session_update(session_id) + + File.write!(notebook_path, "# Sibling notebook") + + assert {:error, {:live_redirect, %{to: session_path}}} = + live(conn, "/sessions/#{session_id}/notebook.livemd") + + assert {:error, {:live_redirect, %{to: ^session_path}}} = + live(conn, "/sessions/#{session_id}/notebook.livemd") + end + + @tag :tmp_dir + test "handles nested paths", %{conn: conn, session_id: session_id, tmp_dir: tmp_dir} do + parent_path = Path.join(tmp_dir, "parent.livemd") + child_dir = Path.join(tmp_dir, "dir") + child_path = Path.join(child_dir, "child.livemd") + + Session.set_path(session_id, parent_path) + wait_for_session_update(session_id) + + File.mkdir!(child_dir) + File.write!(child_path, "# Child notebook") + + {:ok, view, _} = + conn + |> live("/sessions/#{session_id}/dir/child.livemd") + |> follow_redirect(conn) + + assert render(view) =~ "Child notebook" + end + + @tag :tmp_dir + test "handles parent paths", %{conn: conn, session_id: session_id, tmp_dir: tmp_dir} do + parent_path = Path.join(tmp_dir, "parent.livemd") + child_dir = Path.join(tmp_dir, "dir") + child_path = Path.join(child_dir, "child.livemd") + + File.mkdir!(child_dir) + Session.set_path(session_id, child_path) + wait_for_session_update(session_id) + + File.write!(parent_path, "# Parent notebook") + + {:ok, view, _} = + conn + |> live("/sessions/#{session_id}/__parent__/parent.livemd") + |> follow_redirect(conn) + + assert render(view) =~ "Parent notebook" + end + end + # Helpers defp wait_for_session_update(session_id) do