diff --git a/CODESMELL.md b/CODESMELL.md index 1f17a38..63f3250 100644 --- a/CODESMELL.md +++ b/CODESMELL.md @@ -151,14 +151,19 @@ --- -### CSM-008 — DB Queries During Render Path -- **Files:** `lib/bds/desktop/shell_live/panel_renderer.ex`, `lib/bds/desktop/shell_live/tab_helpers.ex` -- **What:** - - `panel_renderer.ex:199-209` — `post_link_entries/1` calls `PostLinks.list_incoming_links/1` and `Posts.get_post/1` per link during render. - - `panel_renderer.ex:229-240` — `git_log_entries/1` calls `Git.file_history/2` and `Posts.get_post/1` during render. - - `tab_helpers.ex:120-135` — `derived_tab_meta/1` queries posts, media, scripts, templates, chat conversations, and import definitions from within the render-prep path. -- **Fix:** Move all data fetching into event handlers (`handle_event`) or the decomposed `reload_shell` updaters from CSM-007. Pre-compute link entries and git log entries when a tab becomes active and store them in assigns. Render should be a pure function of assigns. -- **Test:** Render a panel 10 times; assert no DB queries fire on re-renders. +### ~~CSM-008 — DB Queries During Render Path~~ ✅ FIXED +- **Fixed:** 2026-05-09 +- **What was done:** + - **Panel renderer** (`lib/bds/desktop/shell_live/panel_renderer.ex`): + - `render_post_links` and `render_git_log` no longer call DB functions during render. Instead they read from pre-computed assigns (`panel_post_links`, `panel_git_entries`). + - Renamed `post_link_entries/1` → `fetch_post_link_entries/1` and `git_log_entries/1` → `fetch_git_log_entries/1`, made them public for use by event handlers. + - **Shell LiveView** (`lib/bds/desktop/shell_live.ex`): + - Added `refresh_panel_data/1` that fetches panel data (post links or git log) based on the active panel tab and stores results in assigns. + - `refresh_layout/2` detects when `current_tab` or `panel.active_tab` changed and calls `refresh_panel_data/1` only when stale — no DB queries on re-renders. + - Initialized `panel_post_links` and `panel_git_entries` assigns in mount. + - **Tab meta** (`lib/bds/desktop/shell_live/tab_helpers.ex`): + - `sync_tab_meta` now skips `derived_tab_meta` DB queries when existing meta already has both title and subtitle populated (`meta_complete?/1` guard). + - Added 5 tests in `test/bds/csm008_render_path_test.exs`: post_links re-render (0 queries), git_log re-render (0 queries), output panel switch (0 queries), tasks panel switch (0 queries), tab meta skip for complete meta (0 queries). --- @@ -425,6 +430,7 @@ - [ ] All high-severity items (CSM-006 to CSM-010) have been addressed. - CSM-006: Fixed. Batch INSERT for reindexing, preloaded post records for rendering. - CSM-007: Fixed. Decomposed into refresh_layout, refresh_sidebar, refresh_content, reload_shell. + - CSM-008: Fixed. Panel data pre-computed in event handlers, tab meta skips DB for complete entries. - [x] CSM-001 fix covers ALL 6 affected files, not just `import_definitions.ex`. - [x] CSM-003 fix covers ALL `Repo.delete!` call sites (posts, tags, scripts, media, projects, templates, translations). - [x] CSM-007 decomposition is the prerequisite for fixing CSM-008 (render-path queries). diff --git a/lib/bds/desktop/shell_live.ex b/lib/bds/desktop/shell_live.ex index d0e1c5f..f8d8702 100644 --- a/lib/bds/desktop/shell_live.ex +++ b/lib/bds/desktop/shell_live.ex @@ -33,6 +33,7 @@ defmodule BDS.Desktop.ShellLive do alias BDS.Desktop.ShellLive.{ ChatSurface, Layout, + PanelRenderer, SessionUtil, ShellCommandRunner, SidebarCreate, @@ -171,6 +172,8 @@ defmodule BDS.Desktop.ShellLive do |> assign(:chat_editor_request_refs, %{}) |> assign(:shell_overlay, nil) |> assign(:output_entries, []) + |> assign(:panel_post_links, %{backlinks: [], outlinks: []}) + |> assign(:panel_git_entries, []) |> reload_shell(workbench) |> tap(&sync_menu_bar_locale/1)} end @@ -567,22 +570,60 @@ defmodule BDS.Desktop.ShellLive do page_language = socket.assigns[:page_language] || ShellData.ui_language() offline_mode = Map.get(socket.assigns, :offline_mode, true) sidebar_data = socket.assigns[:sidebar_data] || %{} + current_tab = current_tab(workbench) + prev_tab = socket.assigns[:current_tab] + prev_panel_tab = + case socket.assigns[:workbench] do + %Workbench{panel: %{active_tab: tab}} -> tab + _ -> nil + end + + socket = + socket + |> assign(:workbench, workbench) + |> assign(:activity_buttons, activity_buttons) + |> assign( + :sidebar_header, + active_sidebar_label(activity_buttons, workbench.active_view, sidebar_data) + ) + |> assign(:panel_tabs, ShellData.panel_tabs(workbench)) + |> assign(:current_tab, current_tab) + |> assign(:editor_meta, ShellData.editor_meta(task_status)) + |> assign( + :status, + ShellData.status_bar(workbench, task_status, dashboard, + ui_language: page_language, + offline_mode: offline_mode + ) + ) + + if panel_data_stale?(current_tab, prev_tab, workbench.panel.active_tab, prev_panel_tab) do + refresh_panel_data(socket) + else + socket + end + end + + defp panel_data_stale?(current_tab, prev_tab, panel_tab, prev_panel_tab) do + current_tab != prev_tab or panel_tab != prev_panel_tab + end + + defp refresh_panel_data(socket) do + panel_tab = socket.assigns.workbench.panel.active_tab socket - |> assign(:workbench, workbench) - |> assign(:activity_buttons, activity_buttons) |> assign( - :sidebar_header, - active_sidebar_label(activity_buttons, workbench.active_view, sidebar_data) + :panel_post_links, + if(panel_tab == :post_links, + do: PanelRenderer.fetch_post_link_entries(socket.assigns), + else: socket.assigns[:panel_post_links] || %{backlinks: [], outlinks: []} + ) ) - |> assign(:panel_tabs, ShellData.panel_tabs(workbench)) - |> assign(:current_tab, current_tab(workbench)) - |> assign(:editor_meta, ShellData.editor_meta(task_status)) |> assign( - :status, - ShellData.status_bar(workbench, task_status, dashboard, - ui_language: page_language, - offline_mode: offline_mode + :panel_git_entries, + if(panel_tab == :git_log, + do: PanelRenderer.fetch_git_log_entries(socket.assigns), + else: socket.assigns[:panel_git_entries] || [] ) ) end diff --git a/lib/bds/desktop/shell_live/panel_renderer.ex b/lib/bds/desktop/shell_live/panel_renderer.ex index 0136370..411aee7 100644 --- a/lib/bds/desktop/shell_live/panel_renderer.ex +++ b/lib/bds/desktop/shell_live/panel_renderer.ex @@ -105,12 +105,12 @@ defmodule BDS.Desktop.ShellLive.PanelRenderer do end defp render_post_links(assigns) do - links = post_link_entries(assigns) + panel_links = assigns[:panel_post_links] || %{backlinks: [], outlinks: []} assigns = assigns - |> assign(:backlinks, Map.get(links, :backlinks, [])) - |> assign(:outlinks, Map.get(links, :outlinks, [])) + |> assign(:backlinks, Map.get(panel_links, :backlinks, [])) + |> assign(:outlinks, Map.get(panel_links, :outlinks, [])) ~H""" <%= if Enum.empty?(@backlinks) and Enum.empty?(@outlinks) do %> @@ -161,7 +161,7 @@ defmodule BDS.Desktop.ShellLive.PanelRenderer do end defp render_git_log(assigns) do - entries = git_log_entries(assigns) + entries = assigns[:panel_git_entries] || [] assigns = assign(assigns, :git_entries, entries) ~H""" @@ -196,7 +196,7 @@ defmodule BDS.Desktop.ShellLive.PanelRenderer do """ end - defp post_link_entries(assigns) do + def fetch_post_link_entries(assigns) do case assigns.current_tab do %{type: :post, id: post_id} -> %{ @@ -226,7 +226,7 @@ defmodule BDS.Desktop.ShellLive.PanelRenderer do |> Enum.reject(&is_nil/1) end - defp git_log_entries(assigns) do + def fetch_git_log_entries(assigns) do case git_history_target(assigns.current_tab) do nil -> [] diff --git a/lib/bds/desktop/shell_live/tab_helpers.ex b/lib/bds/desktop/shell_live/tab_helpers.ex index 4c7d97c..f691586 100644 --- a/lib/bds/desktop/shell_live/tab_helpers.ex +++ b/lib/bds/desktop/shell_live/tab_helpers.ex @@ -30,7 +30,13 @@ defmodule BDS.Desktop.ShellLive.TabHelpers do Enum.reduce(tabs, %{}, fn tab, acc -> key = {tab.type, tab.id} existing_meta = Map.get(tab_meta, key, %{}) - synced_meta = merge_missing_meta(existing_meta, derived_tab_meta(tab)) + + synced_meta = + if meta_complete?(existing_meta) do + existing_meta + else + merge_missing_meta(existing_meta, derived_tab_meta(tab)) + end if map_size(synced_meta) == 0 do acc @@ -198,6 +204,12 @@ defmodule BDS.Desktop.ShellLive.TabHelpers do defp derived_tab_meta(_tab), do: %{} + defp meta_complete?(%{title: title, subtitle: subtitle}) + when is_binary(title) and title != "" and is_binary(subtitle) and subtitle != "", + do: true + + defp meta_complete?(_), do: false + defp merge_missing_meta(existing_meta, fresh_meta) do existing_meta |> maybe_put_missing(:title, Map.get(fresh_meta, :title)) diff --git a/test/bds/csm008_render_path_test.exs b/test/bds/csm008_render_path_test.exs new file mode 100644 index 0000000..18233a5 --- /dev/null +++ b/test/bds/csm008_render_path_test.exs @@ -0,0 +1,179 @@ +defmodule BDS.CSM008RenderPathTest 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) + + temp_dir = + Path.join(System.tmp_dir!(), "bds-csm008-#{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: "CSM008", data_path: temp_dir}) + + {:ok, post} = + BDS.Posts.create_post(%{ + project_id: project.id, + title: "CSM008 Test Post", + slug: "csm008-test", + status: :draft, + language: "en" + }) + + %{project: project, post: post} + end + + describe "panel re-renders fire no DB queries" do + test "post_links panel re-render uses cached assigns", %{post: post} do + {:ok, view, _html} = live_isolated(build_conn(), BDS.Desktop.ShellLive) + + render_click(view, "select_tab", %{"type" => "post", "id" => post.id}) + render_click(view, "select_panel_tab", %{"tab" => "post_links"}) + + query_count = count_queries(fn -> + 1..10 |> Enum.each(fn _ -> render(view) end) + end) + + assert query_count == 0, + "Expected 0 DB queries on panel re-renders, got #{query_count}" + end + + test "git_log panel re-render uses cached assigns", %{post: post} do + {:ok, view, _html} = live_isolated(build_conn(), BDS.Desktop.ShellLive) + + render_click(view, "select_tab", %{"type" => "post", "id" => post.id}) + render_click(view, "select_panel_tab", %{"tab" => "git_log"}) + + query_count = count_queries(fn -> + 1..10 |> Enum.each(fn _ -> render(view) end) + end) + + assert query_count == 0, + "Expected 0 DB queries on git_log re-renders, got #{query_count}" + end + end + + describe "select_panel_tab triggers panel data fetch only for active panel" do + test "switching to output panel triggers no post/media queries" do + {:ok, view, _html} = live_isolated(build_conn(), BDS.Desktop.ShellLive) + + {query_count, query_sources} = count_queries_with_sources(fn -> + render_click(view, "select_panel_tab", %{"tab" => "output"}) + end) + + refute "post_links" in query_sources, + "Switching to output should not query post_links, got: #{inspect(query_sources)}" + + assert query_count == 0, + "Expected 0 queries for output panel tab, got #{query_count}" + end + + test "switching to tasks panel triggers no post/media queries" do + {:ok, view, _html} = live_isolated(build_conn(), BDS.Desktop.ShellLive) + + {query_count, _query_sources} = count_queries_with_sources(fn -> + render_click(view, "select_panel_tab", %{"tab" => "tasks"}) + end) + + assert query_count == 0, + "Expected 0 queries for tasks panel tab, got #{query_count}" + end + end + + describe "sync_tab_meta skips DB queries for complete meta" do + test "tabs with existing title and subtitle skip DB queries", %{post: post} do + {:ok, view, _html} = live_isolated(build_conn(), BDS.Desktop.ShellLive) + + render_click(view, "open_sidebar_item", %{ + "route" => "post", + "id" => post.id, + "title" => "Preset Title", + "subtitle" => "preset subtitle" + }) + + query_count = count_queries(fn -> + render_click(view, "select_tab", %{"type" => "post", "id" => post.id}) + end) + + assert query_count == 0, + "Expected 0 DB queries when tab meta is already complete, got #{query_count}" + end + end + + defp count_queries(func) do + test_pid = self() + ref = make_ref() + handler_id = "csm008-query-counter-#{inspect(ref)}" + + :telemetry.attach( + handler_id, + [:bds, :repo, :query], + fn _event, _measurements, _metadata, _ -> + send(test_pid, {:query_executed, ref}) + end, + nil + ) + + func.() + + :telemetry.detach(handler_id) + count_messages(ref, 0) + end + + defp count_queries_with_sources(func) do + test_pid = self() + ref = make_ref() + handler_id = "csm008-query-sources-#{inspect(ref)}" + + :telemetry.attach( + handler_id, + [:bds, :repo, :query], + fn _event, _measurements, metadata, _ -> + source = metadata[:source] || extract_table(metadata[:query] || "") + send(test_pid, {:query_executed, ref, source}) + end, + nil + ) + + func.() + + :telemetry.detach(handler_id) + collect_query_sources(ref, 0, []) + end + + defp collect_query_sources(ref, count, sources) do + receive do + {:query_executed, ^ref, source} -> + collect_query_sources(ref, count + 1, [source | sources]) + after + 0 -> {count, Enum.uniq(sources)} + end + end + + defp extract_table(query) when is_binary(query) do + cond do + String.contains?(query, "post_links") -> "post_links" + String.contains?(query, "posts") -> "posts" + String.contains?(query, "media") -> "media" + String.contains?(query, "scripts") -> "scripts" + String.contains?(query, "templates") -> "templates" + true -> "other" + end + end + + defp extract_table(_), do: "unknown" + + defp count_messages(ref, acc) do + receive do + {:query_executed, ^ref} -> count_messages(ref, acc + 1) + after + 0 -> acc + end + end +end