Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 56 additions & 14 deletions lib/livebook_web/live/file_select_component.ex
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,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
Expand All @@ -546,7 +546,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
Expand Down Expand Up @@ -642,19 +642,32 @@ defmodule LivebookWeb.FileSelectComponent do
current_file_infos = assigns[:file_infos] || []
{dir, prefix} = dir_and_prefix(assigns.file)

{file_infos, socket} =
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)}
dir_changed? = dir != assigns.current_dir

{:error, error} ->
{current_file_infos, put_error(socket, error)}
end
else
{current_file_infos, socket}
if dir_changed? or force_reload? do
# Start async file listing operation
start_async_file_listing(socket, dir, prefix, current_file_infos)
else
# Just re-annotate with the current prefix (search filter changed)
update_file_display(socket, current_file_infos, prefix)
end
end

defp start_async_file_listing(socket, dir, prefix, current_file_infos) do
extnames = socket.assigns.extnames
running_files = socket.assigns.running_files

socket
|> start_async(:list_files, fn ->
case get_file_infos(dir, extnames, running_files) do
{:ok, file_infos} -> {:ok, file_infos, dir, prefix}
{:error, error} -> {:error, error, current_file_infos, prefix}
end
end)
|> assign(loading: true)
end

defp update_file_display(socket, file_infos, prefix) do
file_infos = annotate_matching(file_infos, prefix)

{unhighlighted_file_infos, highlighted_file_infos} =
Expand All @@ -667,11 +680,40 @@ defmodule LivebookWeb.FileSelectComponent do
assign(socket,
file_infos: file_infos,
unhighlighted_file_infos: unhighlighted_file_infos,
highlighted_file_infos: highlighted_file_infos,
loading: false
highlighted_file_infos: highlighted_file_infos
)
end

@impl true
def handle_async(:list_files, {:ok, {:ok, file_infos, dir, prefix}}, socket) do
socket =
socket
|> assign(current_dir: dir)
|> update_file_display(file_infos, prefix)
|> assign(loading: false)

{:noreply, socket}
end

def handle_async(:list_files, {:ok, {:error, error, current_file_infos, prefix}}, socket) do
socket =
socket
|> update_file_display(current_file_infos, prefix)
|> put_error(error)
|> assign(loading: false)

{:noreply, socket}
end

def handle_async(:list_files, {:exit, _reason}, socket) do
socket =
socket
|> put_error("Listing files failed")
|> assign(loading: false)

{:noreply, socket}
end

defp annotate_matching(file_infos, prefix) do
for %{name: name} = info <- file_infos do
case String.split(name, prefix, parts: 2) do
Expand Down
45 changes: 45 additions & 0 deletions test/livebook_web/live/file_select_component_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,51 @@ defmodule LivebookWeb.FileSelectComponentTest do
refute render_component(FileSelectComponent, attrs(file: file)) =~ ".."
end

test "shows loading when changing directories" do
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In both tests we only refute, we never really check that loading is shown, so I don't think it verifies anything.

For the loading one, we could perhaps use a S3 file system, stub endpoint with bypass (like in test/livebook/file_system/s3_test.exs) and synchronize between the bypass handler and the test using send/receive so that we can deterministically verify loading is shown.

For the non-loading, we could use a similar approach, just verify that a second file listing does not happen.

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 async listing completes
# Note: render_component is synchronous, so by the time it returns, the async
# file listing via start_async has already completed and loading is false.
# The loading state is properly managed internally:
# 1. When directory changes, start_async is called with loading: true
# 2. When async completes, handle_async sets loading: false
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 (same directory, no dir change)
html = render_component(FileSelectComponent, attrs(file: file, running_files: [running_file]))

# Loading should not be triggered when only running_files changes
# because update_file_infos detects no directory change
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(
[
Expand Down