From 7464aa6d555e6c906ed388ffbc384822bc980b33 Mon Sep 17 00:00:00 2001 From: Dalton Alexandre <166029845+dl-alexandre@users.noreply.github.com> Date: Sun, 22 Mar 2026 13:23:25 -0700 Subject: [PATCH 1/4] fix: only show loading in FileSelectComponent when listing files Moves loading state management from event handlers into update_file_infos/2 to ensure loading indicator only appears when actually fetching directory listings, not when running_files changes (e.g., notebooks start/stop). - Remove upfront loading assignments in set_file_system and set_path - Set loading only when dir != current_dir in update_file_infos - Add tests for loading behavior with directory vs running_files changes Fixes #3111 --- .../live/file_select_component.ex | 11 +++- .../live/file_select_component_test.exs | 55 +++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/lib/livebook_web/live/file_select_component.ex b/lib/livebook_web/live/file_select_component.ex index dcdef0cf223..0e81d708fd4 100644 --- a/lib/livebook_web/live/file_select_component.ex +++ b/lib/livebook_web/live/file_select_component.ex @@ -523,7 +523,7 @@ defmodule LivebookWeb.FileSelectComponent do send_event(socket.assigns.target, {:mount_file_system, file_system}) send_event(socket.assigns.target, {:set_file, file, %{exists: true}}) - {:noreply, assign(socket, loading: true)} + {:noreply, socket} end def handle_event("set_path", %{"path" => path}, socket) do @@ -544,7 +544,7 @@ defmodule LivebookWeb.FileSelectComponent do end send_event(socket.assigns.target, {:set_file, file, info}) - {:noreply, assign(socket, loading: socket.assigns.file.path != path)} + {:noreply, socket} end def handle_event("clear_error", %{}, socket) do @@ -640,8 +640,13 @@ defmodule LivebookWeb.FileSelectComponent do current_file_infos = assigns[:file_infos] || [] {dir, prefix} = dir_and_prefix(assigns.file) + dir_changed? = dir != assigns.current_dir + + # Only show loading when changing directories (which requires listing files) + socket = if dir_changed?, do: assign(socket, :loading, true), else: socket + {file_infos, socket} = - if dir != assigns.current_dir or force_reload? do + if dir_changed? or force_reload? do case get_file_infos(dir, assigns.extnames, assigns.running_files) do {:ok, file_infos} -> {file_infos, assign(socket, :current_dir, dir)} diff --git a/test/livebook_web/live/file_select_component_test.exs b/test/livebook_web/live/file_select_component_test.exs index 946e38330e9..87b58fe98b8 100644 --- a/test/livebook_web/live/file_select_component_test.exs +++ b/test/livebook_web/live/file_select_component_test.exs @@ -23,6 +23,61 @@ defmodule LivebookWeb.FileSelectComponentTest do refute render_component(FileSelectComponent, attrs(file: file)) =~ ".." end + test "shows loading when changing directories" do + file = FileSystem.File.local(notebooks_path() <> "/") + + # Initial render in one directory + html = + render_component( + FileSelectComponent, + attrs(file: file, id: "test-loading-dir-change") + ) + + # Loading should not be shown after initial render + refute html =~ ~s(role="status") + + # Simulate changing to a different directory via send_update with a new file + new_file = FileSystem.File.local(p("/")) + + html = + render_component( + FileSelectComponent, + attrs(file: new_file, id: "test-loading-dir-change") + ) + + # After changing directory, loading should be reset to false after listing + refute html =~ ~s(role="status") + end + + test "does not show loading when only running_files changes" do + file = FileSystem.File.local(notebooks_path() <> "/") + running_file = FileSystem.File.local(notebooks_path() <> "/basic.livemd") + + # Initial render with no running files + html = + render_component( + FileSelectComponent, + attrs(file: file, id: "test-loading-running", running_files: []) + ) + + refute html =~ ~s(role="status") + # Verify basic.livemd is shown but not marked as running + assert html =~ "basic.livemd" + refute html =~ "play-circle-line" + + # Update with running_files changed + html = + render_component( + FileSelectComponent, + attrs(file: file, id: "test-loading-running", running_files: [running_file]) + ) + + # Loading should not be shown when only running_files changes + refute html =~ ~s(role="status") + # Verify basic.livemd is now marked as running + assert html =~ "play-circle-line" + end + defp attrs(attrs) do Keyword.merge( [ From da38026eab7c8043ebfd4b170264a20cd5cc2972 Mon Sep 17 00:00:00 2001 From: Dalton Alexandre <166029845+dl-alexandre@users.noreply.github.com> Date: Mon, 23 Mar 2026 06:59:11 -0700 Subject: [PATCH 2/4] refactor: remove redundant id from test attrs --- .../live/file_select_component_test.exs | 24 ++++--------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/test/livebook_web/live/file_select_component_test.exs b/test/livebook_web/live/file_select_component_test.exs index 87b58fe98b8..b5986cb0a17 100644 --- a/test/livebook_web/live/file_select_component_test.exs +++ b/test/livebook_web/live/file_select_component_test.exs @@ -27,11 +27,7 @@ defmodule LivebookWeb.FileSelectComponentTest do file = FileSystem.File.local(notebooks_path() <> "/") # Initial render in one directory - html = - render_component( - FileSelectComponent, - attrs(file: file, id: "test-loading-dir-change") - ) + html = render_component(FileSelectComponent, attrs(file: file)) # Loading should not be shown after initial render refute html =~ ~s(role="status") @@ -39,11 +35,7 @@ defmodule LivebookWeb.FileSelectComponentTest do # Simulate changing to a different directory via send_update with a new file new_file = FileSystem.File.local(p("/")) - html = - render_component( - FileSelectComponent, - attrs(file: new_file, id: "test-loading-dir-change") - ) + html = render_component(FileSelectComponent, attrs(file: new_file)) # After changing directory, loading should be reset to false after listing refute html =~ ~s(role="status") @@ -54,11 +46,7 @@ defmodule LivebookWeb.FileSelectComponentTest do running_file = FileSystem.File.local(notebooks_path() <> "/basic.livemd") # Initial render with no running files - html = - render_component( - FileSelectComponent, - attrs(file: file, id: "test-loading-running", running_files: []) - ) + html = render_component(FileSelectComponent, attrs(file: file, running_files: [])) refute html =~ ~s(role="status") # Verify basic.livemd is shown but not marked as running @@ -66,11 +54,7 @@ defmodule LivebookWeb.FileSelectComponentTest do refute html =~ "play-circle-line" # Update with running_files changed - html = - render_component( - FileSelectComponent, - attrs(file: file, id: "test-loading-running", running_files: [running_file]) - ) + html = render_component(FileSelectComponent, attrs(file: file, running_files: [running_file])) # Loading should not be shown when only running_files changes refute html =~ ~s(role="status") From 58521a58de99e857c13c8e43e0f5e63030d150ae Mon Sep 17 00:00:00 2001 From: Dalton Alexandre <166029845+dl-alexandre@users.noreply.github.com> Date: Mon, 23 Mar 2026 08:23:15 -0700 Subject: [PATCH 3/4] fix: prevent scroll jump when clicking on cell output Add scroll-margin CSS to data-el-cell-body to reduce scroll jumps when the cell body receives focus (e.g., when clicking on cell output). Add mousedown handler in cell.js to preserve scroll position when clicking on non-editor areas of the cell body. --- assets/css/js_interop.css | 6 ++++++ assets/js/hooks/cell.js | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/assets/css/js_interop.css b/assets/css/js_interop.css index 34d9992949e..82df88133cb 100644 --- a/assets/css/js_interop.css +++ b/assets/css/js_interop.css @@ -6,6 +6,12 @@ we hide/show certain elements. This way we don't have to engage the server in solely client-side operations. */ +/* Prevent scroll jump when cell body is focused (e.g., clicking on output) */ +[data-el-cell-body] { + scroll-margin-top: 50px; + scroll-margin-bottom: 50px; +} + /* Hooks */ [phx-hook="Dropzone"][data-js-dragging] { diff --git a/assets/js/hooks/cell.js b/assets/js/hooks/cell.js index 9699e455877..f438f668652 100644 --- a/assets/js/hooks/cell.js +++ b/assets/js/hooks/cell.js @@ -68,6 +68,26 @@ const Cell = { this.el.setAttribute("data-js-hover", ""); }); + // Prevent scroll jump when clicking on cell output by handling mousedown + // on the cell body. This ensures clicking on output doesn't trigger + // unwanted scroll behavior when the cell body receives focus. + const cellBody = this.el.querySelector(`[data-el-cell-body]`); + if (cellBody) { + cellBody.addEventListener("mousedown", (event) => { + // If clicking on output (not an editor), prevent default focus scroll + if (!event.target.closest(`[data-el-editor-container]`)) { + // Save scroll position before focus changes + const scrollX = window.scrollX; + const scrollY = window.scrollY; + + // After focus is applied, restore scroll position + requestAnimationFrame(() => { + window.scrollTo(scrollX, scrollY); + }); + } + }); + } + this.el.addEventListener("mouseleave", (event) => { this.el.removeAttribute("data-js-hover"); }); From 6a405b2a979cc5c896371939b979628eb20d9277 Mon Sep 17 00:00:00 2001 From: Dalton Alexandre <166029845+dl-alexandre@users.noreply.github.com> Date: Sun, 29 Mar 2026 19:05:47 -0700 Subject: [PATCH 4/4] fix: remove file_select_component changes that belong to PR #3157 --- .../live/file_select_component.ex | 11 ++---- .../live/file_select_component_test.exs | 39 ------------------- 2 files changed, 3 insertions(+), 47 deletions(-) diff --git a/lib/livebook_web/live/file_select_component.ex b/lib/livebook_web/live/file_select_component.ex index 0e81d708fd4..dcdef0cf223 100644 --- a/lib/livebook_web/live/file_select_component.ex +++ b/lib/livebook_web/live/file_select_component.ex @@ -523,7 +523,7 @@ defmodule LivebookWeb.FileSelectComponent do send_event(socket.assigns.target, {:mount_file_system, file_system}) send_event(socket.assigns.target, {:set_file, file, %{exists: true}}) - {:noreply, socket} + {:noreply, assign(socket, loading: true)} end def handle_event("set_path", %{"path" => path}, socket) do @@ -544,7 +544,7 @@ defmodule LivebookWeb.FileSelectComponent do end send_event(socket.assigns.target, {:set_file, file, info}) - {:noreply, socket} + {:noreply, assign(socket, loading: socket.assigns.file.path != path)} end def handle_event("clear_error", %{}, socket) do @@ -640,13 +640,8 @@ defmodule LivebookWeb.FileSelectComponent do current_file_infos = assigns[:file_infos] || [] {dir, prefix} = dir_and_prefix(assigns.file) - dir_changed? = dir != assigns.current_dir - - # Only show loading when changing directories (which requires listing files) - socket = if dir_changed?, do: assign(socket, :loading, true), else: socket - {file_infos, socket} = - if dir_changed? or force_reload? do + if dir != assigns.current_dir or force_reload? do case get_file_infos(dir, assigns.extnames, assigns.running_files) do {:ok, file_infos} -> {file_infos, assign(socket, :current_dir, dir)} diff --git a/test/livebook_web/live/file_select_component_test.exs b/test/livebook_web/live/file_select_component_test.exs index b5986cb0a17..946e38330e9 100644 --- a/test/livebook_web/live/file_select_component_test.exs +++ b/test/livebook_web/live/file_select_component_test.exs @@ -23,45 +23,6 @@ defmodule LivebookWeb.FileSelectComponentTest do refute render_component(FileSelectComponent, attrs(file: file)) =~ ".." end - test "shows loading when changing directories" do - file = FileSystem.File.local(notebooks_path() <> "/") - - # Initial render in one directory - html = render_component(FileSelectComponent, attrs(file: file)) - - # Loading should not be shown after initial render - refute html =~ ~s(role="status") - - # Simulate changing to a different directory via send_update with a new file - new_file = FileSystem.File.local(p("/")) - - html = render_component(FileSelectComponent, attrs(file: new_file)) - - # After changing directory, loading should be reset to false after listing - refute html =~ ~s(role="status") - end - - test "does not show loading when only running_files changes" do - file = FileSystem.File.local(notebooks_path() <> "/") - running_file = FileSystem.File.local(notebooks_path() <> "/basic.livemd") - - # Initial render with no running files - html = render_component(FileSelectComponent, attrs(file: file, running_files: [])) - - refute html =~ ~s(role="status") - # Verify basic.livemd is shown but not marked as running - assert html =~ "basic.livemd" - refute html =~ "play-circle-line" - - # Update with running_files changed - html = render_component(FileSelectComponent, attrs(file: file, running_files: [running_file])) - - # Loading should not be shown when only running_files changes - refute html =~ ~s(role="status") - # Verify basic.livemd is now marked as running - assert html =~ "play-circle-line" - end - defp attrs(attrs) do Keyword.merge( [