chore: more work on code smell

Co-authored-by: Copilot <copilot@github.com>
2026-04-30 21:41:12 +02:00
parent 6b7603b1cf
commit fc25154d1c
6 changed files with 155 additions and 59 deletions
--- a/CODESMELL.md
+++ b/CODESMELL.md
@@ -261,7 +261,7 @@ This suggests data isn't normalized at boundaries. Prefer atoms for internal str
 1. **Add `@spec` to public APIs of core contexts.** ✅ **DONE 2026-04-30.** See "Priority #1 Completion" section below.
 2. **Extract filesystem / Search side effects out of `Repo.transaction` in `BDS.Media`.** ✅ **DONE 2026-04-30.** See "Priority #2 Completion" section below.
 3. **Fix `MCP.atomize_keys`** to use `String.to_existing_atom/1` with a string-fallback. ✅ **DONE 2026-04-30.** See "Priority #3 Completion" section below.
-4. **Introduce `BDS.PostMedia` Ecto schema** and migrate the 6–8 raw `post_media` queries. Direct type-safety win.
+4. **Introduce `BDS.PostMedia` Ecto schema** and migrate the 6–8 raw `post_media` queries. ✅ **DONE 2026-04-30.** See "Priority #4 Completion" section below.
 5. **Replace `Repo.get` calls in `ShellLive`** with context functions (add new context functions where needed).
 6. **Move locale from `Process.put` into assigns**, then ban `Process.put` via Credo.
 7. **Extract shared helpers** (`attr/2`, `maybe_put/3`, `blank_to_nil/1`, `progress_callback/1`, rebuild progress reporters) into `BDS.MapUtils` / `BDS.ProgressReporter`.
@@ -375,6 +375,38 @@ The codebase has nine other `String.to_atom/1` call sites; all of them operate o

 ---

+## Priority #4 Completion (2026-04-30)
+
+Introduced [lib/bds/posts/post_media.ex](lib/bds/posts/post_media.ex) — a proper `Ecto.Schema` for the `post_media` join table — and migrated every raw SQL / string-table query in the codebase to use it.
+
+**New schema:** `BDS.Posts.PostMedia` with fields `id`, `project_id`, `post_id`, `media_id`, `sort_order`, `created_at`, `belongs_to :post` and `belongs_to :media` associations, full `@type t`, and a `changeset/2` enforcing `validate_required` + `foreign_key_constraint` + `unique_constraint([:post_id, :media_id])`.
+
+**Call sites migrated:**
+
+- [lib/bds/media.ex](lib/bds/media.ex)
+  - `list_linked_posts/1` — replaced `join: post_media in "post_media"` string-table join with `join: pm in PostMedia`.
+  - `link_media_to_post/2` — replaced `Repo.query("SELECT 1 FROM post_media …")` with `Repo.exists?(from pm in PostMedia, …)` and `Repo.query("INSERT INTO post_media …")` with `%PostMedia{} |> changeset() |> Repo.insert!()` (uniqueness now enforced through the schema constraint).
+  - `unlink_media_from_post/2` — replaced `Repo.query("DELETE FROM post_media …")` with `Repo.delete_all(from pm in PostMedia, …)`.
+  - `linked_post_ids/1` — replaced raw `SELECT post_id` with `Repo.all(from pm in PostMedia, select: pm.post_id)`.
+  - `next_sort_order/1` — replaced raw `SELECT COALESCE(MAX(sort_order), -1)` with `Repo.one(from pm in PostMedia, select: max(pm.sort_order))` and a `value when is_integer(value)` guard for the empty-table case.
+- [lib/bds/posts.ex](lib/bds/posts.ex)
+  - `linked_media_ids/1` — replaced raw `SELECT media_id` with `Repo.all(from pm in PostMedia, select: pm.media_id)`.
+- [lib/bds/desktop/shell_live/post_editor.ex](lib/bds/desktop/shell_live/post_editor.ex)
+  - `linked_media/1` — replaced raw `SELECT media_id, sort_order` with a `Repo.all(from pm in PostMedia, select: {pm.media_id, pm.sort_order})` query.
+- [lib/bds/desktop/shell_live/overlay_components.ex](lib/bds/desktop/shell_live/overlay_components.ex)
+  - `post_media_ids/1` — replaced raw `SELECT media_id` with `Repo.all(from pm in PostMedia, select: pm.media_id)`.
+  - `delete_details/2` — replaced the raw `SELECT posts.title FROM posts JOIN post_media ON …` with a typed Ecto join query (`from post in Post, join: pm in PostMedia, on: pm.post_id == post.id, …`).
+
+**Why this matters:**
+
+- All `post_media` access now goes through a typed schema, so every column reference is checked at compile time by Ecto and any future migration that renames a column will produce a `Postgrex/Sqlite` error at compile or test time instead of silently breaking at runtime.
+- The unique `(post_id, media_id)` constraint is now enforceable via `Ecto.Changeset.unique_constraint/2`, which would let the next refactor turn `link_media_to_post/2` into an idempotent upsert without losing the protection.
+- `Repo.query/2` is reserved for the few remaining cases that genuinely need raw SQL (FTS5 virtual tables in `BDS.Search`, which are not Ecto-mappable).
+
+**Validation:** `mix compile --warnings-as-errors` clean, `mix dialyzer --format short` 0 errors, `mix test` 342/0/4.
+
+---
+
 ## Bottom Line

 The biggest risks are **module size** and **duplicated helpers**, followed by the **process dictionary i18n** and **side effects in transactions**. Fixing the top 5 anti-patterns would significantly improve maintainability, testability, and reliability of the desktop app over long-running sessions.
--- a/lib/bds/desktop/shell_live/overlay_components.ex
+++ b/lib/bds/desktop/shell_live/overlay_components.ex
@@ -9,7 +9,7 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
  alias BDS.{I18n, Metadata, Repo}
  alias BDS.Media.Media
  alias BDS.Media.Translation, as: MediaTranslation
-  alias BDS.Posts.{Post, Translation}
+  alias BDS.Posts.{Post, PostMedia, Translation}
  alias BDS.Tags.Tag

  embed_templates "overlay_html/*"
@@ -112,10 +112,12 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
  end

  defp post_media_ids(%{type: :post, id: post_id}) do
-    case Repo.query("SELECT media_id FROM post_media WHERE post_id = ? ORDER BY sort_order ASC, media_id ASC", [post_id]) do
-      {:ok, %{rows: rows}} -> Enum.map(rows, fn [media_id] -> media_id end)
-      _other -> []
-    end
+    Repo.all(
+      from pm in PostMedia,
+        where: pm.post_id == ^post_id,
+        order_by: [asc: pm.sort_order, asc: pm.media_id],
+        select: pm.media_id
+    )
  rescue
    _error -> []
  end
@@ -229,10 +231,15 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do
      end

    reference_list =
-      case Repo.query("SELECT posts.title FROM posts JOIN post_media ON posts.id = post_media.post_id WHERE post_media.media_id = ? ORDER BY post_media.sort_order ASC, posts.updated_at DESC", [media_id]) do
-        {:ok, %{rows: rows}} -> Enum.map(rows, fn [title] -> title || media_id end)
-        _other -> []
-      end
+      Repo.all(
+        from post in Post,
+          join: pm in PostMedia,
+          on: pm.post_id == post.id,
+          where: pm.media_id == ^media_id,
+          order_by: [asc: pm.sort_order, desc: post.updated_at],
+          select: post.title
+      )
+      |> Enum.map(&(&1 || media_id))

    %{
      title: ShellData.translate("Delete Media", %{}, page_language),
--- a/lib/bds/desktop/shell_live/post_editor.ex
+++ b/lib/bds/desktop/shell_live/post_editor.ex
@@ -8,7 +8,7 @@ defmodule BDS.Desktop.ShellLive.PostEditor do
  alias BDS.Desktop.ShellData
  alias BDS.{AI, I18n, Metadata, PostLinks, Posts, Preview, Repo, Tags, Templates}
  alias BDS.Media.Media
-  alias BDS.Posts.{Post, Translation}
+  alias BDS.Posts.{Post, PostMedia, Translation}
  alias BDS.UI.Workbench

  embed_templates "post_editor_html/*"
@@ -770,27 +770,29 @@ defmodule BDS.Desktop.ShellLive.PostEditor do
  end

  defp linked_media(post_id) do
-    case Repo.query("SELECT media_id, sort_order FROM post_media WHERE post_id = ? ORDER BY sort_order ASC, media_id ASC", [post_id]) do
-      {:ok, %{rows: rows}} ->
-        Enum.map(rows, fn [media_id, sort_order] ->
-          case Repo.get(Media, media_id) do
-            %Media{} = media ->
-              %{
-                media_id: media.id,
-                has_thumbnail: String.starts_with?(to_string(media.mime_type || ""), "image/"),
-                name: media.title || media.original_name || media.id,
-                sort_order: sort_order || 0
-              }
+    rows =
+      Repo.all(
+        from pm in PostMedia,
+          where: pm.post_id == ^post_id,
+          order_by: [asc: pm.sort_order, asc: pm.media_id],
+          select: {pm.media_id, pm.sort_order}
+      )

-            _other ->
-              nil
-          end
-        end)
-        |> Enum.reject(&is_nil/1)
+    Enum.map(rows, fn {media_id, sort_order} ->
+      case Repo.get(Media, media_id) do
+        %Media{} = media ->
+          %{
+            media_id: media.id,
+            has_thumbnail: String.starts_with?(to_string(media.mime_type || ""), "image/"),
+            name: media.title || media.original_name || media.id,
+            sort_order: sort_order || 0
+          }

-      _other ->
-        []
-    end
+        _other ->
+          nil
+      end
+    end)
+    |> Enum.reject(&is_nil/1)
  rescue
    _error -> []
  end
--- a/lib/bds/media.ex
+++ b/lib/bds/media.ex
@@ -7,6 +7,7 @@ defmodule BDS.Media do
  alias BDS.Media.Media
  alias BDS.Media.Translation
  alias BDS.Persistence
+  alias BDS.Posts.PostMedia
  alias BDS.Projects
  alias BDS.Rebuild
  alias BDS.Repo
@@ -403,14 +404,14 @@ defmodule BDS.Media do
  def list_linked_posts(media_id) when is_binary(media_id) do
    Repo.all(
      from post in BDS.Posts.Post,
-        join: post_media in "post_media",
-        on: post_media.post_id == post.id,
-        where: post_media.media_id == ^media_id,
-        order_by: [asc: post_media.sort_order, asc: post.updated_at],
+        join: pm in PostMedia,
+        on: pm.post_id == post.id,
+        where: pm.media_id == ^media_id,
+        order_by: [asc: pm.sort_order, asc: post.updated_at],
        select: %{
          post_id: post.id,
          title: fragment("COALESCE(?, ?, ?)", post.title, post.slug, post.id),
-          sort_order: post_media.sort_order
+          sort_order: pm.sort_order
        }
    )
  end
@@ -429,20 +430,23 @@ defmodule BDS.Media do
        project = Projects.get_project!(media.project_id)

        case Repo.transaction(fn ->
-               case Repo.query("SELECT 1 FROM post_media WHERE post_id = ? AND media_id = ? LIMIT 1", [post.id, media.id]) do
-                 {:ok, %{rows: [[1]]}} ->
-                   :already_linked
+               if Repo.exists?(from pm in PostMedia, where: pm.post_id == ^post.id and pm.media_id == ^media.id) do
+                 :already_linked
+               else
+                 sort_order = next_sort_order(media.id)

-                 _other ->
-                   sort_order = next_sort_order(media.id)
+                 %PostMedia{}
+                 |> PostMedia.changeset(%{
+                   id: Ecto.UUID.generate(),
+                   project_id: media.project_id,
+                   post_id: post.id,
+                   media_id: media.id,
+                   sort_order: sort_order,
+                   created_at: Persistence.now_ms()
+                 })
+                 |> Repo.insert!()

-                   {:ok, _result} =
-                     Repo.query(
-                       "INSERT INTO post_media (id, project_id, post_id, media_id, sort_order, created_at) VALUES (?, ?, ?, ?, ?, ?)",
-                       [Ecto.UUID.generate(), media.project_id, post.id, media.id, sort_order, Persistence.now_ms()]
-                     )
-
-                   :linked
+                 :linked
               end
             end) do
          {:ok, _result} ->
@@ -466,8 +470,10 @@ defmodule BDS.Media do
        project = Projects.get_project!(media.project_id)

        case Repo.transaction(fn ->
-               {:ok, _result} =
-                 Repo.query("DELETE FROM post_media WHERE media_id = ? AND post_id = ?", [media.id, post_id])
+               {_count, _} =
+                 Repo.delete_all(
+                   from pm in PostMedia, where: pm.media_id == ^media.id and pm.post_id == ^post_id
+                 )

               :ok
             end) do
@@ -908,15 +914,21 @@ defmodule BDS.Media do
  end

  defp linked_post_ids(media_id) do
-    case Repo.query("SELECT post_id FROM post_media WHERE media_id = ? ORDER BY sort_order ASC, post_id ASC", [media_id]) do
-      {:ok, %{rows: rows}} -> Enum.map(rows, fn [post_id] -> post_id end)
-      {:error, _reason} -> []
-    end
+    Repo.all(
+      from pm in PostMedia,
+        where: pm.media_id == ^media_id,
+        order_by: [asc: pm.sort_order, asc: pm.post_id],
+        select: pm.post_id
+    )
  end

  defp next_sort_order(media_id) do
-    case Repo.query("SELECT COALESCE(MAX(sort_order), -1) FROM post_media WHERE media_id = ?", [media_id]) do
-      {:ok, %{rows: [[value]]}} when is_integer(value) -> value + 1
+    case Repo.one(
+           from pm in PostMedia,
+             where: pm.media_id == ^media_id,
+             select: max(pm.sort_order)
+         ) do
+      value when is_integer(value) -> value + 1
      _other -> 0
    end
  end
--- a/lib/bds/posts.ex
+++ b/lib/bds/posts.ex
@@ -13,6 +13,7 @@ defmodule BDS.Posts do
  alias BDS.PostLinks
  alias BDS.Posts.Link
  alias BDS.Posts.Post
+  alias BDS.Posts.PostMedia
  alias BDS.Posts.Translation
  alias BDS.Projects
  alias BDS.Rebuild
@@ -1411,10 +1412,12 @@ defmodule BDS.Posts do
  end

  defp linked_media_ids(post_id) do
-    case Repo.query("SELECT media_id FROM post_media WHERE post_id = ? ORDER BY sort_order ASC, media_id ASC", [post_id]) do
-      {:ok, %{rows: rows}} -> Enum.map(rows, fn [media_id] -> media_id end)
-      {:error, _reason} -> []
-    end
+    Repo.all(
+      from pm in PostMedia,
+        where: pm.post_id == ^post_id,
+        order_by: [asc: pm.sort_order, asc: pm.media_id],
+        select: pm.media_id
+    )
  end

  defp sync_deleted_post_media_sidecar(media_id) do
--- a/lib/bds/posts/post_media.ex
+++ b/lib/bds/posts/post_media.ex
@@ -0,0 +1,40 @@
+defmodule BDS.Posts.PostMedia do
+  @moduledoc false
+
+  use Ecto.Schema
+  import Ecto.Changeset
+
+  @primary_key {:id, :string, autogenerate: false}
+  @foreign_key_type :string
+
+  @type t :: %__MODULE__{
+          id: String.t() | nil,
+          project_id: String.t() | nil,
+          post_id: String.t() | nil,
+          media_id: String.t() | nil,
+          post: term(),
+          media: term(),
+          sort_order: integer() | nil,
+          created_at: integer() | nil
+        }
+
+  schema "post_media" do
+    field :project_id, :string
+
+    belongs_to :post, BDS.Posts.Post, foreign_key: :post_id, references: :id, type: :string
+    belongs_to :media, BDS.Media.Media, foreign_key: :media_id, references: :id, type: :string
+
+    field :sort_order, :integer, default: 0
+    field :created_at, :integer
+  end
+
+  @spec changeset(t() | Ecto.Changeset.t(), map()) :: Ecto.Changeset.t()
+  def changeset(post_media, attrs) do
+    post_media
+    |> cast(attrs, [:id, :project_id, :post_id, :media_id, :sort_order, :created_at])
+    |> validate_required([:id, :project_id, :post_id, :media_id, :sort_order, :created_at])
+    |> foreign_key_constraint(:post_id)
+    |> foreign_key_constraint(:media_id)
+    |> unique_constraint([:post_id, :media_id], name: :post_media_post_media_idx)
+  end
+end