fix: fix CSM-012

CODESMELL.md

@@ -212,13 +212,15 @@
 
 ---
 
-### CSM-012 — Desktop File Dialog Blocks Event Handler
+### ~~CSM-012 — Desktop File Dialog Blocks Event Handler~~ ✅ FIXED
-- **File:** `lib/bds/desktop/shell_live/sidebar_create.ex:44-69`
+- **Fixed:** 2026-05-09
-- **What:** `FilePicker.choose_file/1` is called directly inside `handle_event`.
+- **What was done:**
-- **Why it's bad:** Can freeze the socket process while the native dialog is open.
+  - Replaced synchronous `FilePicker.choose_file/1` call in `SidebarCreate.create/4` for the "media" kind with `Task.async`, storing the task ref in a new `file_picker_task` socket assign.
-- **Mitigating factor:** In a desktop app, the user flow naturally waits for the dialog result. The risk is low in practice.
+  - Added `handle_file_picker_result/2` private function in `ShellLive` with clauses for `{:ok, _media}`, `:cancel`, `{:error, %{message: _}}`, and `{:error, reason}`.
-- **Fix:** Spawn a short-lived `Task` or use Desktop library's non-blocking async APIs.
+  - Extended the existing `handle_info({ref, result}, socket)` and `handle_info({:DOWN, ref, ...}, socket)` handlers to match on `file_picker_task` ref.
-- **Test:** Trigger a file picker; send another event immediately; assert the second event is handled within 100ms.
+  - Added `BDS_DESKTOP_AUTOMATION` guard to `FilePicker.choose_file/1` — returns `:cancel` immediately in automation/test mode, preventing native dialogs from opening during tests.
+  - Initialized `file_picker_task: nil` assign in mount.
+  - Added 5 tests in `test/bds/csm012_file_picker_async_test.exs`: event handler returns within 100ms, LiveView handles other events while task is pending, task completion doesn't crash LiveView, cancel is handled gracefully, error results don't crash LiveView.
 
 ---
 

lib/bds/desktop/file_picker.ex

@@ -2,11 +2,15 @@ defmodule BDS.Desktop.FilePicker do
   @moduledoc false
 
   def choose_file(prompt) when is_binary(prompt) do
+    if System.get_env("BDS_DESKTOP_AUTOMATION") == "1" do
+      :cancel
+    else
       case :os.type() do
         {:unix, :darwin} -> choose_file_macos(prompt)
         _other -> {:error, %{message: "File selection is only supported on macOS desktop"}}
       end
     end
+  end
 
   defp choose_file_macos(prompt) do
     script = "POSIX path of (choose file with prompt \"#{escape_applescript(prompt)}\")"

lib/bds/desktop/shell_live.ex

@@ -170,6 +170,7 @@ defmodule BDS.Desktop.ShellLive do
      |> assign(:sidebar_filters_by_view, %{})
      |> assign(:sidebar_filter_panels, %{})
      |> assign(:chat_editor_request_refs, %{})
+     |> assign(:file_picker_task, nil)
      |> assign(:shell_overlay, nil)
      |> assign(:output_entries, [])
      |> assign(:panel_post_links, %{backlinks: [], outlinks: []})
@@ -508,6 +509,12 @@ defmodule BDS.Desktop.ShellLive do
     Process.demonitor(ref, [:flush])
 
     cond do
+      socket.assigns.file_picker_task == ref ->
+        {:noreply,
+         socket
+         |> assign(:file_picker_task, nil)
+         |> handle_file_picker_result(result)}
+
       Map.has_key?(socket.assigns.chat_editor_request_refs, ref) ->
         {conversation_id, remaining_refs} = Map.pop(socket.assigns.chat_editor_request_refs, ref)
 
@@ -527,6 +534,21 @@ defmodule BDS.Desktop.ShellLive do
   def handle_info({:DOWN, ref, :process, _pid, reason}, socket) when is_reference(ref) do
     next_socket =
       cond do
+        socket.assigns.file_picker_task == ref ->
+          if reason == :normal do
+            assign(socket, :file_picker_task, nil)
+          else
+            socket
+            |> assign(:file_picker_task, nil)
+            |> append_output_entry(
+              dgettext("ui", "Import media"),
+              inspect(reason),
+              nil,
+              "error"
+            )
+            |> refresh_content(socket.assigns.workbench)
+          end
+
         Map.has_key?(socket.assigns.chat_editor_request_refs, ref) ->
           {conversation_id, remaining_refs} =
             Map.pop(socket.assigns.chat_editor_request_refs, ref)
@@ -839,6 +861,23 @@ defmodule BDS.Desktop.ShellLive do
   defp create_sidebar_item(socket, kind),
     do: SidebarCreate.create(socket, kind, sidebar_create_callbacks())
 
+  defp handle_file_picker_result(socket, {:ok, _media}),
+    do: refresh_content(socket, socket.assigns.workbench)
+
+  defp handle_file_picker_result(socket, :cancel), do: socket
+
+  defp handle_file_picker_result(socket, {:error, %{message: message}}),
+    do:
+      socket
+      |> append_output_entry(dgettext("ui", "Import media"), message, nil, "error")
+      |> refresh_content(socket.assigns.workbench)
+
+  defp handle_file_picker_result(socket, {:error, reason}),
+    do:
+      socket
+      |> append_output_entry(dgettext("ui", "Import media"), inspect(reason), nil, "error")
+      |> refresh_content(socket.assigns.workbench)
+
   defp sidebar_create_callbacks do
     %{
       reload: &reload_shell/2,

lib/bds/desktop/shell_live/sidebar_create.ex

@@ -1,6 +1,8 @@
 defmodule BDS.Desktop.ShellLive.SidebarCreate do
   @moduledoc false
 
+  import Phoenix.Component, only: [assign: 3]
+
   alias BDS.Desktop.{FilePicker}
   alias BDS.AI
   alias BDS.ImportDefinitions
@@ -41,32 +43,24 @@ defmodule BDS.Desktop.ShellLive.SidebarCreate do
     end
   end
 
-  def create(socket, project_id, "media", callbacks) do
+  def create(socket, project_id, "media", _callbacks) do
-    case FilePicker.choose_file(dgettext("ui", "Import media")) do
+    prompt = dgettext("ui", "Import media")
-      {:ok, source_path} ->
-        case BDS.Media.import_media(%{project_id: project_id, source_path: source_path}) do
-          {:ok, _media} ->
-            callbacks.refresh_content.(socket, socket.assigns.workbench)
 
-          {:error, reason} ->
+    task =
-            socket
+      Task.async(fn ->
-            |> callbacks.append_output.(
+        case FilePicker.choose_file(prompt) do
-              dgettext("ui", "Import media"),
+          {:ok, source_path} ->
-              inspect(reason),
+            BDS.Media.import_media(%{project_id: project_id, source_path: source_path})
-              nil,
-              "error"
-            )
-            |> callbacks.refresh_content.(socket.assigns.workbench)
-        end
 
           :cancel ->
-        callbacks.refresh_content.(socket, socket.assigns.workbench)
+            :cancel
 
-      {:error, %{message: message}} ->
+          {:error, reason} ->
-        socket
+            {:error, reason}
-        |> callbacks.append_output.(dgettext("ui", "Import media"), message, nil, "error")
-        |> callbacks.refresh_content.(socket.assigns.workbench)
         end
+      end)
+
+    assign(socket, :file_picker_task, task.ref)
   end
 
   def create(socket, project_id, "script", callbacks) do

test/bds/csm012_file_picker_async_test.exs

@@ -0,0 +1,91 @@
+defmodule BDS.CSM012FilePickerAsyncTest do
+  use ExUnit.Case, async: false
+
+  import Phoenix.ConnTest
+  import Phoenix.LiveViewTest
+
+  @endpoint BDS.Desktop.Endpoint
+
+  setup do
+    :ok = Ecto.Adapters.SQL.Sandbox.checkout(BDS.Repo)
+
+    prev = System.get_env("BDS_DESKTOP_AUTOMATION")
+    System.put_env("BDS_DESKTOP_AUTOMATION", "1")
+
+    on_exit(fn ->
+      if prev,
+        do: System.put_env("BDS_DESKTOP_AUTOMATION", prev),
+        else: System.delete_env("BDS_DESKTOP_AUTOMATION")
+    end)
+
+    temp_dir =
+      Path.join(System.tmp_dir!(), "bds-csm012-#{System.unique_integer([:positive])}")
+
+    File.mkdir_p!(temp_dir)
+    on_exit(fn -> File.rm_rf(temp_dir) end)
+
+    {:ok, project} = BDS.Projects.create_project(%{name: "CSM012", data_path: temp_dir})
+    %{project: project}
+  end
+
+  describe "file picker does not block the event handler" do
+    test "create media returns within 100ms — event handler is not blocked" do
+      {:ok, view, _html} = live(build_conn(), "/")
+
+      start_time = System.monotonic_time(:millisecond)
+      render_click(view, "create_sidebar_item", %{"kind" => "media"})
+      elapsed = System.monotonic_time(:millisecond) - start_time
+
+      assert elapsed < 100, "create_sidebar_item should return within 100ms, took #{elapsed}ms"
+    end
+
+    test "LiveView handles other events while file picker task is pending" do
+      {:ok, view, _html} = live(build_conn(), "/")
+
+      render_click(view, "create_sidebar_item", %{"kind" => "media"})
+
+      start_time = System.monotonic_time(:millisecond)
+      render_click(view, "select_view", %{"view" => "media"})
+      elapsed = System.monotonic_time(:millisecond) - start_time
+
+      assert elapsed < 100, "select_view should respond within 100ms, took #{elapsed}ms"
+    end
+
+    test "task completion does not crash the LiveView" do
+      {:ok, view, _html} = live(build_conn(), "/")
+
+      render_click(view, "create_sidebar_item", %{"kind" => "media"})
+
+      # Wait for the async task to complete (returns :cancel in automation mode)
+      Process.sleep(50)
+
+      assert Process.alive?(view.pid)
+      assert render(view)
+    end
+
+    test "file picker cancel result is handled gracefully" do
+      {:ok, view, _html} = live(build_conn(), "/")
+
+      # In automation mode, FilePicker.choose_file returns :cancel,
+      # so the task result will be :cancel
+      render_click(view, "create_sidebar_item", %{"kind" => "media"})
+      Process.sleep(50)
+
+      # LiveView should still be alive and responsive after cancel
+      assert Process.alive?(view.pid)
+      render_click(view, "select_view", %{"view" => "media"})
+      assert render(view)
+    end
+
+    test "error result from file picker does not crash LiveView" do
+      {:ok, view, _html} = live(build_conn(), "/")
+
+      # Simulate an error result being received
+      send(view.pid, {make_ref(), {:error, %{message: "dialog failed"}}})
+      Process.sleep(10)
+
+      assert Process.alive?(view.pid)
+      assert render(view)
+    end
+  end
+end