hugo/bDS2

Files

Chili Palmer 9a44e6acc8 chore: more on code smells

2026-04-30 18:18:57 +02:00

19 KiB

Raw Blame History

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.

Summary

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.

Critical Anti-Patterns

1. Massive Module Bloat (Most Serious)

Several modules are far too large and violate the single-responsibility principle.

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).

2. Process Dictionary for I18n State

(and 13 other call sites across all *_editor.ex modules — 15 total) In BDS.Desktop.ShellLive:

def render(assigns) do
  Process.put(:bds_ui_locale, assigns.page_language)
  index(assigns)
end

And later:

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.

3. Side Effects Inside Database Transactions

(7 transactions in lib/bds/media.ex follow this pattern) In BDS.Media.import_media/1:

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.

4. `String.to_atom/1` on Untrusted Input

In BDS.MCP:

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.

5. Bang Functions (`File.read!`) in Business Logic Without Rescue

Found in BDS.Posts, BDS.Media, BDS.MCP, BDS.Generation:

defp parse_rebuild_file(project, path) do
  contents = File.read!(path)
  {:ok, %{fields: fields}} = Frontmatter.parse_document(contents)
  ...
end

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)
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.

7. Direct Repo Access in LiveView (10 sites verified)

BDS.Desktop.ShellLive directly queries the repo:

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.

8. `String.to_existing_atom/1` on User Input Without Whitelist

In ShellLive:

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.

9. Nested Exception Handling for Control Flow

In ShellLive:

defp safe_existing_atom(action) when is_binary(action) do
  String.to_existing_atom(action)
rescue
  ArgumentError -> nil
end

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.

10. `Jason.decode!` Without Err and `BDS.AI` (HTTP-response decoding)or Handling

In BDS.AI.OpenAICompatibleRuntime:

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.

Moderate Concerns

11. Missing `@spec` Typespecs

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

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.).

13. Raw SQL Overuse

19 Repo.query sites split into two groups:

FTS5 virtual tables in BDS.Search (posts_fts, media_fts, MATCH, bm25, rank). Ecto cannot express FTS5 queries cleanly. Keep these raw.
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.

14. Atom/String Key Duality

Many functions normalize between atom and string keys repeatedly:

Map.get(assigns, :language, Map.get(assigns, "language", ...))

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)

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.

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.

Priority Order

Add @spec to public APIs of core contexts. ✅ DONE 2026-04-30. See "Priority #1 Completion" section below.
Extract filesystem / Search side effects out of Repo.transaction in BDS.Media. ✅ DONE 2026-04-30. See "Priority #2 Completion" section below.
Fix MCP.atomize_keys to use String.to_existing_atom/1 with a string-fallback. Removes a real atom-table DoS vector.
Introduce BDS.PostMedia Ecto schema and migrate the 6–8 raw post_media queries. Direct type-safety win.
Replace Repo.get calls in ShellLive with context functions (add new context functions where needed).
Move locale from Process.put into assigns, then ban Process.put via Credo.
Extract shared helpers (attr/2, maybe_put/3, blank_to_nil/1, progress_callback/1, rebuild progress reporters) into BDS.MapUtils / BDS.ProgressReporter.
Wrap external Jason.decode! calls in BDS.AI.OpenAICompatibleRuntime and BDS.AI with Jason.decode/1 + {:error, _} propagation.
Module split. BDS.Generation (2624) and BDS.Desktop.ShellLive (2607) first; schedule each as its own sprint.

Skipped / downgraded

#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.

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 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.

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.

19 KiB Raw Blame History Unescape Escape

Elixir Code Smell Analysis — bDS2

Summary

Critical Anti-Patterns

1. Massive Module Bloat (Most Serious)

2. Process Dictionary for I18n State

3. Side Effects Inside Database Transactions

4. String.to_atom/1 on Untrusted Input

5. Bang Functions (File.read!) in Business Logic Without Rescue

6. Duplicate Helper Functions Across Contexts

7. Direct Repo Access in LiveView (10 sites verified)

8. String.to_existing_atom/1 on User Input Without Whitelist

9. Nested Exception Handling for Control Flow

10. Jason.decode! Without Err and BDS.AI (HTTP-response decoding)or Handling

Moderate Concerns

11. Missing @spec Typespecs

12. Tests Run Synchronously

13. Raw SQL Overuse

14. Atom/String Key Duality

15. GenServer State Growth (Tasks)

What the Project Does Well

Priority Order

Skipped / downgraded

Bottom Line

Priority #1 Completion (2026-04-30)

Priority #2 Completion (2026-04-30)

Bottom Line

19 KiB

Raw Blame History

4. `String.to_atom/1` on Untrusted Input

5. Bang Functions (`File.read!`) in Business Logic Without Rescue

8. `String.to_existing_atom/1` on User Input Without Whitelist

10. `Jason.decode!` Without Err and `BDS.AI` (HTTP-response decoding)or Handling

11. Missing `@spec` Typespecs