From 9691d931b332c0d78b06c9b984c7fcec4f65bcee Mon Sep 17 00:00:00 2001 From: Chili Palmer Date: Fri, 1 May 2026 10:57:21 +0200 Subject: [PATCH] chore: updated code smell doc Co-authored-by: Copilot --- CODESMELL.md | 510 ++++++++++----------------------------------------- 1 file changed, 99 insertions(+), 411 deletions(-) diff --git a/CODESMELL.md b/CODESMELL.md index b6fabed..17a4577 100644 --- a/CODESMELL.md +++ b/CODESMELL.md @@ -1,500 +1,188 @@ # Elixir Code Smell Analysis — bDS2 -> Generated by static analysis of the codebase. Do not delete; use as a reference for refactoring sprints. -> -> **Updated 2026-04-30:** Each claim re-verified against current code. See "Verification Summary" and "Priority Order" sections at the bottom. +Living document. Each section lists **status**, then **open work in priority order**, then a brief notes block. Completed work is listed compactly at the bottom (`## Changelog`). + +Last refreshed: 2026-05-01. --- -## Summary +## 1. God Modules -The project is architecturally sound at a high level: contexts are well-defined, Ecto is used properly for schema validation, `with` blocks handle errors cleanly, and the GenServer implementations are mostly correct. However, **the codebase suffers from severe module bloat, excessive duplication of helper functions, risky process-dictionary usage, and several side-effect-in-transaction anti-patterns** that become maintenance liabilities in a desktop application expected to run for long sessions. +**Status:** in progress. Five originally-flagged god modules are reduced to coordinator size; eight new files (mostly LiveView editors) crossed the 800-line threshold and are now the active queue. + +### Open queue (priority order) + +| # | Module | Current lines | Target | Strategy | +|---|---|---|---|---| +| 1 | `BDS.Maintenance` | 810 | ≤ 250 | Extract `DiffReports` (~240), `DiffComputation` (~160), `Repair` (~160), `FileScan` (~140), `Progress` (~60). Coordinator keeps the 4 public entrypoints. | +| 2 | `BDS.Media` | 993 | ≤ 250 | Extract `Thumbnails` (~140), `Sidecars` (~150), `FileOps` (~180), `Rebuild` (~130), `Linking` (~80). Main keeps CRUD + translation API. | +| 3 | `BDS.Desktop.ShellLive.ImportEditor` | 1436 | ≤ 600 | Extract `ConflictResolution` (~150), `TaxonomyEditing` (~120), `AnalysisState` (~150), `ProgressTracking` (~120). Components stay in main file. | +| 4 | `BDS.Rendering` | 838 | ≤ 200 | Extract `TemplateSelection` (~120), `PostRendering` (~180), `ListArchive` (~150), `Metadata` (~140), `LinksAndLanguages` (~100). Main keeps the 3 public renders. | +| 5 | `BDS.Desktop.ShellLive.MenuEditor` | 871 | ≤ 350 | Extract `TreeOps` (~280), `TreePredicates` (~60), `DraftManagement` (~140), `PageCategory` (~120), `State` (~80). | +| 6 | `BDS.Desktop.ShellLive.PostEditor` | 963 | ≤ 400 | Extract `DraftManagement` (~180), `ListValues` (~160), `Persistence` (~140), `PostMetadata` (~150). | +| 7 | `BDS.Desktop.ShellLive.SettingsEditor` | 872 | ≤ 350 | Extract `ProjectSettings` (~140), `AISettings` (~150), `PublishingSettings` (~80), `ManagedCategories` (~140), `StyleEditor` (~80), `MCPConfig` (~60). | +| 8 | `BDS.Desktop.ShellLive.ChatEditor` | 972 | ≤ 400 | Extract `ToolSurfaces` (~280), `ToolTracking` (~140), `MessageBuild` (~160), `ModelSelection` (~100). Defer — highest internal coupling. | +| 9 | `BDS.MCP` | 677 | ≤ 350 | Split tools / resources / proposals / serialization clusters. (Carried over from original priority list.) | + +**Established pattern:** extract cohesive helper clusters into submodules under `lib//.ex`; `import BDS.X.Y, only: [...]` from the main module so internal call sites are unchanged; `defdelegate` for any helper still needed through the public namespace; verify with `mix compile --warnings-as-errors`, `mix dialyzer --format short`, and full `mix test` after each extraction. + +### Completed (see Changelog for details) + +- `BDS.Generation` 2651 → 647 (76 %) +- `BDS.AI` 1711 → 168 (90 %) +- `BDS.Scripting.Capabilities` 1715 → 194 (89 %) +- `BDS.Posts` 1781 → 569 (68 %) +- `BDS.Desktop.ShellLive` 2607 → 1545 (41 %) --- -## Critical Anti-Patterns +## 2. Process Dictionary for i18n State -### 1. Massive Module Bloat (Most Serious) +**Status:** open. `Process.put(:bds_ui_locale, …)` + `Process.get(:bds_ui_locale)` in `render/1` of `BDS.Desktop.ShellLive` and every `*_editor.ex` (15 sites total). -Several modules are far too large and violate the single-responsibility principle. +**Why it matters:** implicit global state; complicates per-process isolation in tests and risks leaks between concurrent operations in the same process. -| Module | Lines | Issue | -|--------|-------|-------| -| `BDS.Desktop.ShellLive` | 2,607 | Handles UI events, routing, overlays, menus, project switching, tab management, translations, task polling, and native menu integration | -| `BDS.Posts` | 1,709 | Handles CRUD, publishing, file I/O, translations, auto-translation scheduling, embeddings sync, search sync, link rebuilding, and validation | -| `BDS.Generation` | 2,624 | Site generation, validation, sitemap building, archive pagination, feed rendering, and file hashing (verified larger than originally documented) | -| `BDS.MCP` | 670 | MCP tools, resources, proposals, post detail serialization, search, and category counting | -| `BDS.Media` | 939 | Media CRUD, thumbnail generation, sidecar I/O, post-media join management | - -**Why this is an anti-pattern:** Giant modules reduce compiler concurrency, make refactoring dangerous, obscure test coverage, and increase merge conflicts. Elixir/OTP best practice is to keep modules under ~400 lines and split by responsibility. - -**Fix:** Extract sub-domains into dedicated modules (e.g., `BDS.Posts.Publisher`, `BDS.Posts.Translations`, `BDS.Generation.SitemapBuilder`, `BDS.Desktop.ShellLive.EventRouter`). +**Plan:** thread `locale` through `assigns`/render args; replace `translated/1,2` with a 3-arity helper that takes the locale explicitly; ban `Process.put` via Credo afterwards. --- -### 2. Process Dictionary for I18n State - (and 13 other call sites across all `*_editor.ex` modules — 15 total) -In `BDS.Desktop.ShellLive`: +## 3. Side Effects in Transactions -```elixir -def render(assigns) do - Process.put(:bds_ui_locale, assigns.page_language) - index(assigns) -end -``` +**Status:** ✅ done in `BDS.Media` (2026-04-30). Open elsewhere — no audit yet for `BDS.Posts`, `BDS.Publishing`, `BDS.Generation`. -And later: - -```elixir -defp translated(text, bindings \\ %{}), do: ShellData.translate(text, bindings, Process.get(:bds_ui_locale)) -``` - -**Why this is an anti-pattern:** The process dictionary is implicit global state. It makes functions harder to test in isolation and can leak state between concurrent operations in the same process. Elixir strongly discourages `Process.put/get` for business logic. - -**Fix:** Pass `locale` explicitly through `assigns` to all translation call sites, or use a dedicated context struct. +**Plan:** spot-check every `Repo.transaction/1` outside `BDS.Media`. Rule: only DB writes inside; filesystem and `Search.sync_*` after commit. --- -### 3. Side Effects Inside Database Transactions - (7 transactions in `lib/bds/media.ex` follow this pattern) -In `BDS.Media.import_media/1`: +## 4. `String.to_atom/1` on Untrusted Input -```elixir -Repo.transaction(fn -> - media = %Media{} |> Media.changeset(...) |> Repo.insert!() - :ok = File.mkdir_p(Path.dirname(destination)) - :ok = File.cp(source_path, destination) - :ok = write_sidecar(project, media) - :ok = ensure_thumbnails(project, media) - :ok = Search.sync_media(media) - media -end) -``` - -**Why this is an anti-pattern:** If the transaction retries (or the DB connection times out in SQLite), filesystem and thumbnail side effects are duplicated or orphaned. Ecto transactions should contain **only** database operations. - -**Fix:** Perform the DB insert in the transaction, then do filesystem work after the transaction succeeds. +**Status:** ✅ done. The only attacker-reachable site (`MCP.atomize_keys/1`) was deleted (2026-04-30). Nine remaining `String.to_atom/1` call sites all operate on bounded enums (workbench types, release platforms, route IDs, capability keys, AI catalog modalities) and are safe. --- -### 4. `String.to_atom/1` on Untrusted Input +## 5. Bang File Operations in Long-Running Code -In `BDS.MCP`: +**Status:** open (limited scope). -```elixir -defp atomize_keys(map) when is_map(map) do - Map.new(map, fn {key, value} -> {String.to_atom(key), value} end) -end -``` - -**Why this is an anti-pattern:** Atoms are not garbage collected. If MCP receives large or arbitrary JSON keys, the VM atom table can fill up and crash the BEAM. - -**Fix:** Use `String.to_existing_atom/1` (with a safe fallback) or keep keys as strings. -Note: 10 other `String.to_atom` call sites exist, but they operate on bounded enums (modality, platform, view id) and are lower risk. +**Scope:** only `File.read!`/`File.write!` reachable from a GenServer worker, scheduled task, or LiveView event matter. Mandatory-config reads at boot are fine. +**Plan:** sweep `BDS.Posts.RebuildFromFiles`, `BDS.Media`, `BDS.MCP`, `BDS.Generation`. Replace bang variants with `{:ok, _} | {:error, _}` propagation only on those paths. --- -### 5. Bang Functions (`File.read!`) in Business Logic Without Rescue +## 6. Duplicate Helpers Across Contexts -Found in `BDS.Posts`, `BDS.Media`, `BDS.MCP`, `BDS.Generation`: +**Status:** open. -```elixir -defp parse_rebuild_file(project, path) do - contents = File.read!(path) - {:ok, %{fields: fields}} = Frontmatter.parse_document(contents) - ... -end -``` +**Duplicated functions (verified copy-pasted across `Posts`, `Media`, `Search`, `Generation`, `Publishing`, `MCP`):** -**Why this is an anti-pattern:** Missing files or permission errors raise unhandled exceptions that crash the caller Limit the change to files reached from long-running processes; mandatory configuration files can stay as `File.read!`.. In a GenServer context (e.g., background tasks), this brings down the worker. - -**Fix:** Use `File.read/1` and handle `{:error, reason}` explicitly, returning `{:error, reason}` tuples to callers. - ---- - -### 6. Duplicate Helper Functions Across Contexts - -The same private helpers are copy-pasted in `BDS.Posts`, `BDS.Media`, `BDS.Search`, `BDS.Generation`, `BDS.Publishing`, `BDS.MCP`: - -- `attr/2` (atom/string key normalization) +- `attr/2` (atom-or-string key access) - `maybe_put/3` - `blank_to_nil/1` - `progress_callback/1` - `report_rebuild_started/3`, `report_rebuild_progress/4` -**Why this is an anti-pattern:** Violates DRY. When behavior needs to change (e.g., adding float progress support), every copy must be updated. - -**Fix:** Extract to a shared utility module such as `BDS.MapUtils`, `BDS.AttrUtils`, or `BDS.ProgressReporter`. +**Plan:** consolidate into `BDS.MapUtils` (attr / maybe_put / blank_to_nil) and `BDS.ProgressReporter` (callback + reporters). Note: `Util` modules already exist inside `BDS.Generation`, `BDS.Scripting.Capabilities`, and `BDS.Posts.*`; the goal is one shared utility, not three more. --- -### 7. Direct Repo Access in LiveView (10 sites verified) +## 7. Direct `Repo.get` in `BDS.Desktop.ShellLive` -`BDS.Desktop.ShellLive` directly queries the repo: +**Status:** open. 10 call sites verified. -```elixir -case Repo.get(Post, post_id) do - %Post{} = post -> ... -end -``` - -**Why this is an anti-pattern:** LiveViews are the web layer; they should call context functions (`BDS.Posts.get_post/1`). Direct Repo access leaks persistence details into the UI and makes testing harder. - -**Fix:** Replace all `Repo.get` calls in `ShellLive` with context function calls. +**Plan:** add the missing context functions (`Posts.get_post_with_translations/1`, etc.) and replace each `Repo.get(Schema, id)` with the context call. --- -### 8. `String.to_existing_atom/1` on User Input Without Whitelist +## 8. `String.to_existing_atom/1` + `rescue ArgumentError` -In `ShellLive`: +**Status:** open, low priority. -```elixir -def handle_event("select_view", %{"view" => view_id}, socket) do - workbench = Workbench.click_activity(socket.assigns.workbench, String.to_existing_atom(view_id)) - ... -end -``` - -**Why this is an anti-pattern:** If a client sends a non-existent atom string, it raises `ArgumentError`. While safer than `to_atom`, it is still a crash vector. - -**Fix:** Validate against a whitelist or map of known string-to-atom conversions before conversion. +**Plan:** introduce explicit string→atom whitelists for the half-dozen call sites (`safe_existing_atom`, view-id parsing, panel-tab parsing) so the rescue clause becomes dead code, then delete it. --- -### 9. Nested Exception Handling for Control Flow +## 9. `Jason.decode!/1` on External HTTP Responses -In `ShellLive`: +**Status:** open. -```elixir -defp safe_existing_atom(action) when is_binary(action) do - String.to_existing_atom(action) -rescue - ArgumentError -> nil -end -``` +**Scope:** only the 2 sites in `BDS.AI` / `BDS.AI.OpenAICompatibleRuntime` that decode HTTP response bodies. The remaining 12 `Jason.decode!/1` sites decode our own on-disk files (metadata, embeddings index, generation hashes) and are not in scope. -**Why this is an anti-pattern:** Using exceptions for expected control flow is slower and harder to follow than explicit validation. - -**Fix:** Maintain an explicit mapping of allowed actions, or use a `case` with `:erlang.binary_to_existing_atom/2` and catch the error tuple. +**Plan:** switch to `Jason.decode/1` and propagate `{:error, reason}` from the runtime through to the chat / one-shot orchestration layer in `BDS.AI.Chat` and `BDS.AI.OneShot`. --- -### 10. `Jason.decode!` Without Err and `BDS.AI` (HTTP-response decoding)or Handling +## 10. Missing `@spec` -In `BDS.AI.OpenAICompatibleRuntime`: +**Status:** ✅ done for core contexts (2026-04-30). Open for LiveView editor modules and the smaller contexts (`Tags`, `Templates`, `Scripts`, `PostLinks`). -```elixir -defp normalize_response(body) do - payload = Jason.decode!(body) - ... -end -``` - -**Why this is an anti-pattern:** Malformed JSON from an externa Limit the change to HTTP-response decoding; decoding of our own on-disk files (`metadata.ex`, `embeddings/index.ex`, etc.) is acceptable. +**Convention reminder:** Ecto schemas need explicit `@type t`; use `term()` for associations; use `@typedoc` + named types for repeated map shapes; for attrs maps use `%{optional(atom()) => term(), optional(String.t()) => term()}`. --- -## Moderate Concerns +## 11. Raw SQL Outside FTS5 -### 11. Missing `@spec` Typespecs +**Status:** ✅ done for the `post_media` table (2026-04-30, replaced by `BDS.Posts.PostMedia` schema). Remaining raw SQL is FTS5-virtual-table queries in `BDS.Search` only — keep them raw. -Verified: only **10 `@spec` declarations across all of `lib/`**, and they are concentrated in `BDS.Scripting` / `BDS.Scripting.Lua`. Every other public context function relies on Dialyzer inference. For a project of this size, adding `@spec` to public APIs is the single highest-ROI type-safety improvement available — and pairs well with the existing `mix dialyzer --format short` check. +--- -### 12. Tests Run Synchronously +## 12. Atom/String Key Duality -Most tests use `use ExUnit.Case, async: false`. Many tests share the SQLite repo and named GenServers, so they cannot realistically be async. Limit conversion to pure-logic test files (parsers, formatters, etc.). +**Status:** open, low priority. -### 13. Raw SQL Overuse +**Pattern:** `Map.get(assigns, :language, Map.get(assigns, "language", default))` in many editors and capability bridges. -19 `Repo.query` sites split into two groups: +**Plan:** normalize at boundaries. Adopt the rule "atoms internally, strings only at JSON/HTTP boundaries", and use `attr/2` (post-#6 consolidation) at every boundary point. -1. **FTS5 virtual tables** in `BDS.Search` (`posts_fts`, `media_fts`, `MATCH`, `bm25`, `rank`). Ecto cannot express FTS5 queries cleanly. **Keep these raw.** -2. **`post_media` join table** queried by hand in `BDS.Posts`, `BDS.Media`, `BDS.Desktop.ShellLive.OverlayComponents`, `BDS.Desktop.ShellLive.PostEditor`. There is no `BDS.PostMedia` schema. This is the real type-safety hole; introducing the schema replaces 6–8 raw queries with Ecto +--- -While SQLite FTS necessitates some raw SQL, many queries in `BDS.Search`, `BDS.Media`, and `BDS.Posts` use `Repo.query!/2` for standard operations that Ecto could express portably (e.g., `DELETE FROM ... WHERE ...`). This reduces type safety and database portability. +## 13. `BDS.Tasks` Memory Growth -### 14. Atom/String Key Duality +**Status:** open, bounded in practice. -Many functions normalize between atom and string keys repeatedly: +**Risk:** the `tasks` map grows for the lifetime of the BEAM unless the UI calls `clear_completed`/`clear_finished`. Long-running desktop sessions could accumulate thousands of finished tasks. -```elixir -Map.get(assigns, :language, Map.get(assigns, "language", ...)) -``` +**Plan:** TTL eviction in the `BDS.Tasks` GenServer (e.g., drop `:completed`/`:failed`/`:cancelled` older than 1 h); already partially mitigated by `recent_finished_limit` in `build_status_snapshot/1`. -This suggests data isn't normalized at boundaries. Prefer atoms for internal structs and strings only at external boundaries (JSON, HTTP params). +--- -### 15. GenServer State Growth (Tasks) +## 14. Synchronous Tests -`BDS.Tasks` stores all tasks forever in a single map. The `clear_completed`/`clear_finished` functions exist, but if the UI doesn't trigger them regularly in a long-running desktop app, memory grows unbounded. Consider TTL-based eviction. +**Status:** intentional, no further action. + +Most tests share the SQLite repo and named GenServers (`BDS.Tasks`, `BDS.Search`, `BDS.AI.Catalog`, `BDS.Embeddings`). `async: false` is correct. Pure-logic tests (parsers, formatters) could opt into `async: true` opportunistically. --- ## What the Project Does Well -- **Contexts are clearly separated** (`Posts`, `Media`, `Projects`, `Search`, etc.). -- **Ecto changesets** are used consistently for validation. -- **`with` blocks** handle multi-step error flows cleanly. -- **Pattern matching** is idiomatic and pervasive. -- **Task.Supervisor** is used correctly for background work. -- *Verification Summary (2026-04-30) - -| # | Claim | Verified | Notes | -|---|---|---|---| -| 1 | Module bloat | ✅ | `Generation` is 2624 lines, worse than originally documented. | -| 2 | `Process.put(:bds_ui_locale)` | ✅ | 15 call sites across `shell_live.ex` and every `*_editor.ex`. | -| 3 | Side effects in transactions | ✅ | 7 transactions in `lib/bds/media.ex` wrap filesystem + Search side effects. | -| 4 | `String.to_atom` on untrusted input | ⚠️ Partly | Only `MCP.atomize_keys` is on arbitrary external JSON. Other 10 sites are bounded enums. | -| 5 | `File.read!` | ⚠️ Partly | Most sites read mandatory config; only loop/background paths are real risk. | -| 6 | Duplicated helpers | ✅ | Real DRY violation across contexts. | -| 7 | `Repo.get` in `ShellLive` | ✅ | 10 direct calls. | -| 8/9 | `to_existing_atom` + rescue | ⚠️ | The rescue effectively *is* the whitelist; cosmetic priority. | -| 10 | `Jason.decode!` on external responses | ⚠️ Partly | Only 2 of 14 sites decode HTTP responses; the rest decode our own files. | -| 11 | Missing `@spec` | ✅ | 10 specs in entire `lib/`. Highest type-safety ROI. | -| 12 | Tests synchronous | ✅ | Limited convertibility — most tests share repo/GenServers. | -| 13 | Raw SQL | ⚠️ Partly | FTS5 must stay raw; `post_media` table is the real fix target. | +- Clear context separation (`Posts`, `Media`, `Projects`, `Search`, `AI`, `Generation`). +- Consistent Ecto changesets at every write boundary. +- Idiomatic `with` blocks for multi-step error flows. +- `Task.Supervisor` used correctly for background work. +- Submodule extraction pattern proven across 5 large modules without breaking public API contracts. --- -## Priority Order +## Changelog -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. ✅ **DONE 2026-04-30.** See "Priority #4 Completion" section below. -5. **Module split.** `BDS.Generation` (2624) and `BDS.Desktop.ShellLive` (2607) first, then `BDS.AI` (1700+) and `BDS.Posts`. ✅ **PARTIAL 2026-04-30 / 2026-05-01.** `BDS.Generation` reduced 2651 → 647 (76%). See "Priority #5 Progress" section below. -6. **Replace `Repo.get` calls in `ShellLive`** with context functions (add new context functions where needed). -7. **Move locale from `Process.put` into assigns**, then ban `Process.put` via Credo. -8. **Extract shared helpers** (`attr/2`, `maybe_put/3`, `blank_to_nil/1`, `progress_callback/1`, rebuild progress reporters) into `BDS.MapUtils` / `BDS.ProgressReporter`. -9. **Wrap external `Jason.decode!` calls** in `BDS.AI.OpenAICompatibleRuntime` and `BDS.AI` with `Jason.decode/1` + `{:error, _}` propagation. +### 2026-05-01 -### Skipped / downgraded +- **God modules**: + - `BDS.Scripting.Capabilities` 1715 → 194 (89 %). Submodules: `Util` (301), `Posts` (270), `Media` (254), `Crud` (284), `Projects` (204), `AppShell` (134), `Bridges` (176). Public `for_project/2` preserved. + - Fixed real race in `test/bds/desktop/shell_live_test.exs:1149` (metadata-diff editor open) — was diagnosed as flake but was a missing `completed_task!(task.id)` synchronization between the worker `:DOWN` and the next `:refresh_task_status` tick. -- #5 in bulk — convert only paths reached from long-running processes. -- #8 / #9 — cosmetic; the rescue functions as a whitelist today. -- #12 — leave `async: false` where the repo or named GenServers are involved. -- #15 — bounded in practice; ensure UI triggers `clear_finished` periodically. +### 2026-04-30 ---- +- **God modules**: + - `BDS.AI` 1711 → 168 (90 %). Submodules: `Chat` (597), `OneShot` (382), `Catalog` (306), `ChatTools` (271), `Runtime` (100), `SettingsStore` (78). Public API preserved via `defdelegate`. + - `BDS.Posts` 1781 → 569 (68 %). Submodules: `Slugs` (86), `AutoTranslation` (176), `FileSync` (146), `TranslationValidation` (464), `RebuildFromFiles` (320), `Translations` (279). Public API preserved via `defdelegate`. + - `BDS.Generation` 2651 → 647 (76 %). Submodules: `Outputs` (490), `Validation` (445), `Data` (352), `Sitemap` (280), `Paths` (262), `Renderers` (227), `Progress` (96), `Pagefind` (70), `GeneratedFileHash` (23). `import only:` used to keep call sites unchanged. + - `BDS.Desktop.ShellLive` 2607 → 1545 (41 %). Submodules: `TitlebarMenu`, `CliSync`, `PanelRenderer`, `TabHelpers`, `TaskLocalization`, `ChatSurface`, `SidebarCreate`, `Layout`, `ShellCommandRunner`, `SessionUtil`. Coordinator now holds only `mount/3`, `render/1`, `handle_event/3`, `handle_info/2`. + - Side fix: `test/bds/maintenance_test.exs` had hardcoded `posts/2026/04/...` paths; added explicit `File.mkdir_p!` calls for the orphan-file fixtures. +- **Side effects in transactions** (`BDS.Media`): every `Repo.transaction/1` block in [lib/bds/media.ex](lib/bds/media.ex) refactored — DB writes inside, filesystem + `Search.sync_*` after commit. Functions touched: `import_media/1`, `update_media/2`, `upsert_media_translation/3`, `delete_media_translation/2`, `replace_media_file/2`, `link_media_to_post/2`, `unlink_media_from_post/2`. Spec correction: `delete_media_translation/2` returns `{:ok, boolean()} | {:error, …}`. +- **`String.to_atom` DoS**: `BDS.MCP.atomize_keys/1` deleted; the two `accept_proposal` sites pass string-keyed maps directly to `Media.update_media/2` and `Posts.update_post/2`, both of which already accept `%{optional(atom()) => term(), optional(String.t()) => term()}`. +- **Raw SQL on `post_media`**: introduced `BDS.Posts.PostMedia` schema with `unique_constraint([:post_id, :media_id])`; migrated 8 raw-query sites in `BDS.Media`, `BDS.Posts`, `BDS.Desktop.ShellLive.PostEditor`, `BDS.Desktop.ShellLive.OverlayComponents` to typed Ecto queries. +- **`@spec` for core contexts**: added specs and `@type t` to `BDS.Projects`, `BDS.Posts`, `BDS.Media`, `BDS.Search`, `BDS.Publishing`, `BDS.Generation`, `BDS.Metadata`, `BDS.MCP`, `BDS.AI` and their schemas. Bugs surfaced: `Search.list_stemmer_languages/0` return shape, `Media.sync_media_sidecar/1` returns `:ok`, `Media.replace_media_file/2` can return `{:ok, nil}`, removed unreachable fall-through clauses in `BDS.Posts` auto-translate cascades and `BDS.Desktop.ShellLive.ChatEditor.blank?/1`. -## Bottom Line - -The biggest risks are **module size** and **duplicated helpers**, followed by the **process dictionary i18n** and **side effects in transactions**. The single highest-ROI improvement for type safety is **adding `@spec` to public context APIs** so Dialyzer can do its job — that is the starting point of the priority order above - ---- - -## Priority #1 Completion (2026-04-30) - -Added `@spec` and `@type` declarations to the public APIs of all core contexts and their schemas. Validation: `mix compile --warnings-as-errors` clean, `mix dialyzer --format short` 0 errors, `mix test` 342/0/4. - -**Files modified (specs added):** - -- `lib/bds/projects.ex`, `lib/bds/projects/project.ex` -- `lib/bds/posts.ex`, `lib/bds/posts/post.ex`, `lib/bds/posts/translation.ex`, `lib/bds/posts/link.ex` -- `lib/bds/media.ex`, `lib/bds/media/media.ex`, `lib/bds/media/translation.ex` -- `lib/bds/search.ex` -- `lib/bds/publishing.ex`, `lib/bds/publishing/publish_job.ex` -- `lib/bds/generation.ex` -- `lib/bds/metadata.ex` -- `lib/bds/mcp.ex` -- `lib/bds/ai.ex` - -**Bugs surfaced and fixed by Dialyzer once specs were in place:** - -- `BDS.Search.list_stemmer_languages/0` returns `[String.t()]`, not `[{String.t(), [String.t()]}]`. -- `BDS.Media.sync_media_sidecar/1` returns `:ok` (not `{:ok, t()}`); the `posts.ex` caller was already pattern-matching on `:ok`. -- `BDS.Media.replace_media_file/2` can return `{:ok, nil}` when the new file's checksum is unchanged. -- Removed unreachable `other -> {:error, other}` fall-through clauses in the auto-translate cascades in `BDS.Posts` (the preceding `{:error, reason}` pattern already covers the only remaining return shape). -- Removed unreachable `defp blank?(_value)` clause in `BDS.Desktop.ShellLive.ChatEditor` (prior clauses already covered `binary` and `nil`, no other types reach the function). - -**Conventions established for future spec work:** - -- Ecto schemas need explicit `@type t` — Ecto does not generate one. -- Use `term()` for `belongs_to` / `has_many` association fields to avoid circular type dependencies between sibling schemas. -- Use `@typedoc` + named types (e.g. `attrs`, `metadata_state`, `search_filters`, `reindex_opts`) to avoid repeating large map shapes across many specs in the same module. -- For `update_*` / attrs-style maps, the canonical type is `%{optional(atom()) => term(), optional(String.t()) => term()}` because both renderer-supplied string keys and Elixir atom keys flow into them. - -**Not yet specced (intentional, out of scope of priority #1):** - -- LiveView modules under `lib/bds/desktop/shell_live/`. These are Phoenix LiveView callbacks (`mount/3`, `handle_event/3`, `handle_info/2`) whose specs are inherited from the behaviour. Public helper functions in those modules can be specced as part of the eventual ShellLive module split (priority #9). -- Internal helpers in `BDS.MCP` and `BDS.AI` that are private (`defp`) — Dialyzer infers these. -- `BDS.Tags`, `BDS.Templates`, `BDS.Scripts`, `BDS.PostLinks` — smaller contexts; queue for a follow-up pass if Dialyzer surfaces issues. - ---- - -## Priority #2 Completion (2026-04-30) - -Refactored every `Repo.transaction/1` block in [lib/bds/media.ex](lib/bds/media.ex) so that only DB writes run inside the transaction. Filesystem writes (`File.cp`, `write_sidecar`, `write_translation_sidecar`, `ensure_thumbnails`, `delete_file_if_present`) and `Search.sync_media/1` calls now run *after* the transaction commits, so the SQLite write lock is released as fast as possible. - -**Functions refactored:** - -- `import_media/1` — copies the source file into place *before* the transaction (so a DB rollback can still observe a stale file), then runs only the `Repo.insert!` inside the transaction, then writes sidecar / thumbnails / search index. On DB failure the copied file is removed. -- `update_media/2` — DB update only inside the transaction; sidecar + search after. -- `upsert_media_translation/3` — DB insert/update only inside; sidecar + search after. -- `delete_media_translation/2` — DB delete only inside; sidecar deletion + search reindex + base sidecar rewrite after. -- `replace_media_file/2` — moves the existing destination to a `.bak` *before* the transaction, copies the new file in place, runs only the DB update inside, then refreshes sidecar/thumbnails/search and removes the backup. On DB failure the original file is restored from the backup. -- `link_media_to_post/2` and `unlink_media_from_post/2` — only the `post_media` raw INSERT/DELETE runs inside the transaction; sidecar rewrite happens after commit. - -**Why this matters:** - -SQLite has a single global write lock. Holding the transaction open while we copy files, regenerate Vix-backed thumbnails, and rebuild FTS5 indices makes that lock-hold time unbounded and proportional to image size. Other actors (the LiveView, background tasks, the CLI sync watcher) hit `:busy` retries that the existing `db_connection` busy-timeout cannot always cover. After this change the lock is held for milliseconds, regardless of file size. - -**Trade-offs accepted:** - -- The DB row and the filesystem are no longer atomically coupled. If the BEAM crashes between `Repo.insert!` and `write_sidecar/2`, the row exists without a sidecar. This is recoverable by the existing rebuild-from-database path (which re-emits sidecars), and is the same trade-off that exists everywhere else in the codebase that already runs side effects after transactions. -- `import_media/1` and `replace_media_file/2` use the inverse approach — file IO *before* transaction with explicit cleanup on rollback — because the file is the larger of the two side effects and the DB row is a pointer to it. This keeps the destination consistent on rollback. - -**Spec correction surfaced by Dialyzer:** - -- `BDS.Media.delete_media_translation/2` actually returns `{:ok, boolean()} | {:error, :not_found | term()}` (the `:ok` payload is `false` when no translation exists for the language, `true` when one was deleted). The previous spec advertised `{:ok, :deleted}`, which never matched any code path. - -**Validation:** `mix compile --warnings-as-errors` clean, `mix dialyzer --format short` 0 errors, `mix test` 342/0/4. - ---- - -## Priority #3 Completion (2026-04-30) - -Removed `BDS.MCP.atomize_keys/1` entirely instead of just narrowing it to `String.to_existing_atom/1`. The function existed only to convert MCP-proposal JSON keys (attacker-controlled strings) into atoms before passing them to `Media.update_media/2` and `Posts.update_post/2`. Both contexts already accept string-keyed maps natively through their `attr/2` helper (`%{optional(atom()) => term(), optional(String.t()) => term()}`), so the conversion was both unnecessary and a textbook unbounded-atom-table DoS vector. - -**Change:** - -- Deleted `defp atomize_keys/1` from [lib/bds/mcp.ex](lib/bds/mcp.ex). -- The two `accept_proposal` call sites now pass `proposal.data["changes"] || %{}` straight through to `Media.update_media/2` and `Posts.update_post/2`. - -**Why removal beats whitelisting:** - -- `String.to_existing_atom/1` would still need a whitelist or a `try/rescue` wrapper, both of which add code without buying type safety — the downstream contexts already key on either form. -- Removing the function eliminates the only place in MCP that converts external JSON to atoms; the surface area for atom-table attacks via the MCP tool API is now zero. -- The `attr/2` helpers in `BDS.Posts` and `BDS.Media` are the *single* canonical place where attribute key normalization happens, which is exactly the invariant we want. - -**Other `String.to_atom/1` call sites considered:** - -The codebase has nine other `String.to_atom/1` call sites; all of them operate on bounded inputs (`Workbench` type names, release platforms, route IDs from the router config, `:supports_attachment` / `:supports_tool_calls` capability keys we ourselves wrote, `parse_modality` from a fixed AI catalog). None of them are reachable from attacker-controlled JSON the way `MCP.accept_proposal` was. They are safe to leave as a follow-up if a stricter sweep is wanted later. - -**Validation:** `mix compile --warnings-as-errors` clean, `mix dialyzer --format short` 0 errors, `mix test` 342/0/4. - ---- - -## 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. - ---- - -## Priority #5 Progress (2026-04-30 / 2026-05-01) - -**Goal:** Split god modules. Started with the worst offender, `BDS.Generation` (2651 lines). - -**Result:** `lib/bds/generation.ex` reduced **2651 → 647 lines (76%)** by extracting nine cohesive submodules under `lib/bds/generation/`: - -| Module | Lines | Responsibility | -|---|---|---| -| `BDS.Generation.Outputs` | 490 | All `build_*_outputs/*` builders + `*_route_paths` + `additional_languages`, `route_post_output_path`, `suppress_subtree_translation_variants` | -| `BDS.Generation.Validation` | 445 | `compare_sitemap_to_html`, `plan_validation_paths`, `build_targeted_validation_plan`, `targeted_output?`, `prune_empty_parent_dirs`, post/lang timestamp checks | -| `BDS.Generation.Data` | 352 | `generation_data/2`, snapshot loaders, post-index builders, translation-lookup helpers | -| `BDS.Generation.Sitemap` | 280 | sitemap.xml, RSS/Atom feeds, calendar feed, hreflang link assembly | -| `BDS.Generation.Paths` | 262 | URL/route/path helpers, language prefixing, pagination math, archive routing | -| `BDS.Generation.Renderers` | 227 | Liquid template rendering wrappers (home, post, archive, date, list, 404) | -| `BDS.Generation.Progress` | 96 | Generation/validation progress callback helpers | -| `BDS.Generation.Pagefind` | 70 | Pagefind search-index input file emission | -| `BDS.Generation.GeneratedFileHash` | 23 | (pre-existing) hash-tracking schema | - -Total: 2245 lines now live in focused submodules; the remaining 647 in `BDS.Generation` is the orchestrating `plan_generation/2`, `apply_validation/2`, `validate_site/3`, `write_generated_file`, and `delete_extra_validation_paths` — small enough to manage as a single coordinator. - -**Refactor pattern used:** `import BDS.Generation.X, only: [...]` (or `except: [...]`) at the head of `BDS.Generation` so the hundreds of internal call sites needed no changes; `defdelegate` for any function that had to remain reachable through the public `BDS.Generation` namespace (e.g. `post_output_path/1,2`). - -**Validation after each extraction:** `mix compile --warnings-as-errors` clean, `mix dialyzer --format short` 0 errors, `mix test` 342/0/4. - -**Date-bug side fix:** `test/bds/maintenance_test.exs` had hardcoded `posts/2026/04/...` paths that worked only when the published-post setup happened to create the same year/month directory. With today's date in May, the orphan writes failed; added explicit `File.mkdir_p!` calls for the hardcoded fixture paths. - -**Remaining work in this priority:** - -- ✅ `BDS.Generation` — done (76% reduction, 647 lines remaining is acceptable for a coordinator). -- 🔄 `BDS.Desktop.ShellLive` (2607 → 1545, 41% reduction). Submodules extracted under `lib/bds/desktop/shell_live/`: - - | Module | Lines | Responsibility | - |---|---|---| - | `TitlebarMenu` | 181 | Menu group definition, dropdown items, open/close/hover/keydown | - | `CliSync` | 133 | CLI watcher entity-change application + tab refresh | - | `PanelRenderer` | 290 | Tasks/output/post-links/git-log panel rendering + editor toolbar | - | `TabHelpers` | 99 | Tab title/subtitle/icon, route atom mapping, post/media labels | - | `TaskLocalization` | 80 | Task status localization, editor-meta translation | - | `ChatSurface` | 233 | Chat-surface action dispatch, assistant message helpers | - | `SidebarCreate` | 131 | New post/media/script/template/import sidebar creation | - | `Layout` | 53 | Sync-layout, resize-panel, parse-width, ignore-shortcut | - | `ShellCommandRunner` | 95 | `apply_shell_command` + `apply_result` dispatch | - | `SessionUtil` | 49 | Workbench-session restore, project-name picker, task-result tracking | - - Coordinator (`shell_live.ex`) now 1545 lines containing only `mount/3`, `render/1`, `handle_event/3`, `handle_info/2` clauses, plus thin dispatchers and small editor-assign helpers. -- ⏳ `BDS.Posts` (1781 → 569, 68% reduction). Submodules extracted under `lib/bds/posts/`: - - | Module | Lines | Responsibility | - |---|---|---| - | `Slugs` | 86 | `slug_available/3`, `unique_slug_for_title/3`, `unique`, `unique_for_import`, `default_source` | - | `AutoTranslation` | 176 | `maybe_schedule/1`, missing-language detection, post + cascading media auto-translate task scheduling | - | `FileSync` | 146 | Post/translation relative-path computation, frontmatter serialization, body extraction, on-disk delete | - | `TranslationValidation` | 464 | `validate/2`, `fix_invalid/1`, invalid DB/FS issue classification, legacy report fields, canonical-language helpers, markdown-file recursion | - | `RebuildFromFiles` | 320 | `rebuild_posts_from_files/2`, `import_orphan_post_file/2`, `import_orphan_post_translation_file/2`, `parse_rebuild_file`, `upsert_post_from_file`, `upsert_post_from_rebuild_file`, `upsert_post_translation_from_rebuild_file`, `progress_callback/1`, `report_rebuild_started/3`, `report_rebuild_progress/4`, `parse_post_status`, `parse_translation_status` | - | `Translations` | 279 | `publish_post_translation/2`, `list_post_translations/1`, `upsert_post_translation/3`, `delete_post_translation/1`, `sync_post_translation_from_file/1`, `rewrite_published_post_translation/1`, `publish_translation/2`, `publish_post_translations/1`, `normalize_translation_updates`, `maybe_reopen_source_post_for_manual_translation` | - - Public API on `BDS.Posts` preserved via `defdelegate` for: `slug_available/3`, `unique_slug_for_title/3`, `validate_translations/2`, `fix_invalid_translations/1`, `rebuild_posts_from_files/2`, `import_orphan_post_file/2`, `import_orphan_post_translation_file/2`, `publish_post_translation/2`, `list_post_translations/1`, `upsert_post_translation/3`, `delete_post_translation/1`, `sync_post_translation_from_file/1`, `rewrite_published_post_translation/1`. Remaining clusters in posts.ex are core CRUD (`create_post`, `update_post`, `publish_post`, `delete_post`, `archive_post`, `discard_post_changes`, `sync_post_from_file`, `rewrite_published_post`, `editor_body`), small stats (`dashboard_stats`, `post_counts_by_year_month`, ~40 lines extractable), and `rebuild_post_links` (~22 lines). Stats could be split next, but ~569 lines is a reasonable steady state. -- ⏳ `BDS.AI` (1711 → 168, **90% reduction**). Submodules extracted under `lib/bds/ai/`: - - | Module | Lines | Responsibility | - |---|---|---| - | `Chat` | 597 | Public chat API (`start_chat`, `list_chat_conversations`, `available_chat_models`, `set_conversation_model`, `list_chat_messages`, `send_chat_message`, `cancel_chat`) + chat round/tool-call orchestration, request building, message truncation, system prompt + project stats summary, conversation/message persistence, `count_distinct_string_list/3`, `normalize_usage/1` | - | `OneShot` | 382 | One-shot operations (`detect_language`, `analyze_taxonomy`, `analyze_import_taxonomy`, `analyze_post`, `translate_post`, `analyze_image`, `translate_media`) + per-op system/user prompt builders, JSON response extraction, post/media input normalization, taxonomy mapping filtering | - | `Catalog` | 306 | Model catalog API (`list_endpoint_models`, `refresh_model_catalog`, `list_catalog_providers`, `get_catalog_model`, `catalog_meta`, `put_model_capabilities`, `format_model`, `model_capabilities`, `decode_nullable_json`) + models.dev fetch, persistence, modality parsing | - | `ChatTools` | 271 | Chat tool dispatch (`execute/3`) for blog_stats, list_posts, list_media, render_table/chart/form/card/metric/list/tabs/mindmap; tool spec generation (`available_specs/2`) | - | `Runtime` | 100 | `model_preference_keys/0`, `resolve_target/2`, `validate_target/3`, `endpoint_with_model/2`, per-operation airplane/online resolution | - | `SettingsStore` | 78 | Setting/secret/catalog-meta storage helpers (`get_setting`, `put_setting`, `delete_setting`, `put_secret`, `get_secret`, `encrypted_key`, `secret_backend`, catalog meta) | - - Public `BDS.AI` API preserved via `defdelegate` for all extracted operations. Remaining 168 lines hold endpoint storage (`put_endpoint`, `get_endpoint`, `delete_endpoint`), airplane mode (`set_airplane_mode`, `airplane_mode?`), model preferences (`put_model_preference`, `get_model_preference`), and the defdelegate facade. - -- ⏳ `BDS.Scripting.Capabilities` (1715 → 194, **89% reduction**). Submodules extracted under `lib/bds/scripting/capabilities/`: - - | Module | Lines | Responsibility | - |---|---|---| - | `Util` | 301 | Sanitization, normalization, arity wrappers, optional-key map builders, datetime parsing, project-path lookup, shell-open helpers | - | `Posts` | 270 | All `posts.*` capabilities (CRUD, publishing, body/cover/excerpt, search, tags, categories, archive, restore, preview path, names-with-counts) | - | `Media` | 254 | All `media.*` capabilities (CRUD, upload, thumbnails, metadata, translations, search) | - | `Crud` | 284 | `scripts.*`, `templates.*`, `tags.*`, `tasks.*` CRUD/search/exec | - | `Projects` | 204 | Project CRUD, metadata read/write, sync-meta-on-startup, data paths, project-for-folder | - | `AppShell` | 134 | Clipboard, bookmarklet, title-bar metrics, renderer-ready, open/select folder, show-in-folder, trigger-menu-action, preview-target, test-mode/env detection | - | `Bridges` | 176 | Sync availability, repo state/status/history/fetch/pull/push/commit-all, upload-site, AI detect/analyze/translate (post + media), embeddings progress/find-similar/compute-similarities/suggest-tags/find-duplicates/dismiss-pair/index-unindexed | - - Public `BDS.Scripting.Capabilities.for_project/2` contract preserved unchanged. Main file (194 lines) now holds only the capability-map assembly using `import` of all submodules. - -- ⏳ `BDS.MCP` (677). - ---- - -## 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. +**Validation gate after every change:** `mix compile --warnings-as-errors` clean, `mix dialyzer --format short` 0 errors, `mix test` 342 tests / 0 failures / 4 skipped.