fix: fixed CSM-008
This commit is contained in:
22
CODESMELL.md
22
CODESMELL.md
@@ -151,14 +151,19 @@
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
### CSM-008 — DB Queries During Render Path
|
### ~~CSM-008 — DB Queries During Render Path~~ ✅ FIXED
|
||||||
- **Files:** `lib/bds/desktop/shell_live/panel_renderer.ex`, `lib/bds/desktop/shell_live/tab_helpers.ex`
|
- **Fixed:** 2026-05-09
|
||||||
- **What:**
|
- **What was done:**
|
||||||
- `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** (`lib/bds/desktop/shell_live/panel_renderer.ex`):
|
||||||
- `panel_renderer.ex:229-240` — `git_log_entries/1` calls `Git.file_history/2` and `Posts.get_post/1` during render.
|
- `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`).
|
||||||
- `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.
|
- 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.
|
||||||
- **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.
|
- **Shell LiveView** (`lib/bds/desktop/shell_live.ex`):
|
||||||
- **Test:** Render a panel 10 times; assert no DB queries fire on re-renders.
|
- 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.
|
- [ ] 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-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-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-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-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).
|
- [x] CSM-007 decomposition is the prerequisite for fixing CSM-008 (render-path queries).
|
||||||
|
|||||||
@@ -33,6 +33,7 @@ defmodule BDS.Desktop.ShellLive do
|
|||||||
alias BDS.Desktop.ShellLive.{
|
alias BDS.Desktop.ShellLive.{
|
||||||
ChatSurface,
|
ChatSurface,
|
||||||
Layout,
|
Layout,
|
||||||
|
PanelRenderer,
|
||||||
SessionUtil,
|
SessionUtil,
|
||||||
ShellCommandRunner,
|
ShellCommandRunner,
|
||||||
SidebarCreate,
|
SidebarCreate,
|
||||||
@@ -171,6 +172,8 @@ defmodule BDS.Desktop.ShellLive do
|
|||||||
|> assign(:chat_editor_request_refs, %{})
|
|> assign(:chat_editor_request_refs, %{})
|
||||||
|> assign(:shell_overlay, nil)
|
|> assign(:shell_overlay, nil)
|
||||||
|> assign(:output_entries, [])
|
|> assign(:output_entries, [])
|
||||||
|
|> assign(:panel_post_links, %{backlinks: [], outlinks: []})
|
||||||
|
|> assign(:panel_git_entries, [])
|
||||||
|> reload_shell(workbench)
|
|> reload_shell(workbench)
|
||||||
|> tap(&sync_menu_bar_locale/1)}
|
|> tap(&sync_menu_bar_locale/1)}
|
||||||
end
|
end
|
||||||
@@ -567,22 +570,60 @@ defmodule BDS.Desktop.ShellLive do
|
|||||||
page_language = socket.assigns[:page_language] || ShellData.ui_language()
|
page_language = socket.assigns[:page_language] || ShellData.ui_language()
|
||||||
offline_mode = Map.get(socket.assigns, :offline_mode, true)
|
offline_mode = Map.get(socket.assigns, :offline_mode, true)
|
||||||
sidebar_data = socket.assigns[:sidebar_data] || %{}
|
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
|
socket
|
||||||
|> assign(:workbench, workbench)
|
|
||||||
|> assign(:activity_buttons, activity_buttons)
|
|
||||||
|> assign(
|
|> assign(
|
||||||
:sidebar_header,
|
:panel_post_links,
|
||||||
active_sidebar_label(activity_buttons, workbench.active_view, sidebar_data)
|
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(
|
|> assign(
|
||||||
:status,
|
:panel_git_entries,
|
||||||
ShellData.status_bar(workbench, task_status, dashboard,
|
if(panel_tab == :git_log,
|
||||||
ui_language: page_language,
|
do: PanelRenderer.fetch_git_log_entries(socket.assigns),
|
||||||
offline_mode: offline_mode
|
else: socket.assigns[:panel_git_entries] || []
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -105,12 +105,12 @@ defmodule BDS.Desktop.ShellLive.PanelRenderer do
|
|||||||
end
|
end
|
||||||
|
|
||||||
defp render_post_links(assigns) do
|
defp render_post_links(assigns) do
|
||||||
links = post_link_entries(assigns)
|
panel_links = assigns[:panel_post_links] || %{backlinks: [], outlinks: []}
|
||||||
|
|
||||||
assigns =
|
assigns =
|
||||||
assigns
|
assigns
|
||||||
|> assign(:backlinks, Map.get(links, :backlinks, []))
|
|> assign(:backlinks, Map.get(panel_links, :backlinks, []))
|
||||||
|> assign(:outlinks, Map.get(links, :outlinks, []))
|
|> assign(:outlinks, Map.get(panel_links, :outlinks, []))
|
||||||
|
|
||||||
~H"""
|
~H"""
|
||||||
<%= if Enum.empty?(@backlinks) and Enum.empty?(@outlinks) do %>
|
<%= if Enum.empty?(@backlinks) and Enum.empty?(@outlinks) do %>
|
||||||
@@ -161,7 +161,7 @@ defmodule BDS.Desktop.ShellLive.PanelRenderer do
|
|||||||
end
|
end
|
||||||
|
|
||||||
defp render_git_log(assigns) do
|
defp render_git_log(assigns) do
|
||||||
entries = git_log_entries(assigns)
|
entries = assigns[:panel_git_entries] || []
|
||||||
assigns = assign(assigns, :git_entries, entries)
|
assigns = assign(assigns, :git_entries, entries)
|
||||||
|
|
||||||
~H"""
|
~H"""
|
||||||
@@ -196,7 +196,7 @@ defmodule BDS.Desktop.ShellLive.PanelRenderer do
|
|||||||
"""
|
"""
|
||||||
end
|
end
|
||||||
|
|
||||||
defp post_link_entries(assigns) do
|
def fetch_post_link_entries(assigns) do
|
||||||
case assigns.current_tab do
|
case assigns.current_tab do
|
||||||
%{type: :post, id: post_id} ->
|
%{type: :post, id: post_id} ->
|
||||||
%{
|
%{
|
||||||
@@ -226,7 +226,7 @@ defmodule BDS.Desktop.ShellLive.PanelRenderer do
|
|||||||
|> Enum.reject(&is_nil/1)
|
|> Enum.reject(&is_nil/1)
|
||||||
end
|
end
|
||||||
|
|
||||||
defp git_log_entries(assigns) do
|
def fetch_git_log_entries(assigns) do
|
||||||
case git_history_target(assigns.current_tab) do
|
case git_history_target(assigns.current_tab) do
|
||||||
nil ->
|
nil ->
|
||||||
[]
|
[]
|
||||||
|
|||||||
@@ -30,7 +30,13 @@ defmodule BDS.Desktop.ShellLive.TabHelpers do
|
|||||||
Enum.reduce(tabs, %{}, fn tab, acc ->
|
Enum.reduce(tabs, %{}, fn tab, acc ->
|
||||||
key = {tab.type, tab.id}
|
key = {tab.type, tab.id}
|
||||||
existing_meta = Map.get(tab_meta, key, %{})
|
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
|
if map_size(synced_meta) == 0 do
|
||||||
acc
|
acc
|
||||||
@@ -198,6 +204,12 @@ defmodule BDS.Desktop.ShellLive.TabHelpers do
|
|||||||
|
|
||||||
defp derived_tab_meta(_tab), 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
|
defp merge_missing_meta(existing_meta, fresh_meta) do
|
||||||
existing_meta
|
existing_meta
|
||||||
|> maybe_put_missing(:title, Map.get(fresh_meta, :title))
|
|> maybe_put_missing(:title, Map.get(fresh_meta, :title))
|
||||||
|
|||||||
179
test/bds/csm008_render_path_test.exs
Normal file
179
test/bds/csm008_render_path_test.exs
Normal file
@@ -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
|
||||||
Reference in New Issue
Block a user