From 41f4f08831042b343ef0d50db78b95019b4aaedb Mon Sep 17 00:00:00 2001 From: Claudio Ortolina Date: Mon, 4 May 2026 22:32:37 +0100 Subject: [PATCH] Streamline project conventions --- docs/project-conventions.md | 65 +++++++++++-------------------------- 1 file changed, 19 insertions(+), 46 deletions(-) diff --git a/docs/project-conventions.md b/docs/project-conventions.md index ca11de29..5c854cc5 100644 --- a/docs/project-conventions.md +++ b/docs/project-conventions.md @@ -4,12 +4,9 @@ Rules extracted from commit history that are specific to this project and not al ## Commit Messages -- Imperative present tense, single-line, under 60 characters -- Describe intent/behavior, not implementation details -- Reverts use `Revert "Original message"` convention -- NEVER ADD a "Co-Authored-By" reference in the message body -- **When the work maps to a Backlog.md task, prefix the commit subject with the task identifier**, e.g. `ML-3: fix scrobble rule ordering`. The prefix counts toward the 60-character limit. -- **Dependency updates use `Update dependencies` for mix and `Update npm dependencies` for npm.** The subject describes the intent, not the mechanics — never use "bump". List each specific version change in the commit body as `package_name from => to`. +→ See `.claude/skills/git-commit/SKILL.md` for full conventions, checklist, and examples. + +Key rules: imperative present tense, single-line under 60 characters, task ID prefix when the work maps to a Backlog.md task, "Update dependencies" for mix and "Update npm dependencies" for npm, never "Co-Authored-By". ## Workflow @@ -44,61 +41,37 @@ Rules extracted from commit history that are specific to this project and not al ## Template / UI -- **Gettext wraps ALL user-facing strings.** Every commit that adds UI text must also update `.pot`/`.po` files. -- **Dark mode always paired:** `text-zinc-900 dark:text-zinc-100`, `bg-zinc-50 dark:bg-zinc-800`. -- **Wishlisted items get dimmed styling:** `opacity-60 hover:opacity-100 transition-opacity`. -- **Icons inside buttons use the `icon` class** instead of explicit size classes (`h-5 w-5`, `size-3.5`, etc.). The `icon` class is provided by Fluxon and auto-sizes icons based on the button's `size` prop. -- **Artist name display uses the MusicBrainz `joinphrase` field** — never join artist names with a literal `", "`. -- **Charts use CSS Grid and responsive HTML**, not SVG. +→ See `.claude/skills/ui-framework/SKILL.md` for full conventions, component patterns, and Fluxon reference. + +Key rules: Gettext wraps ALL user-facing strings, dark mode always paired (`text-zinc-900 dark:text-zinc-100`), wishlisted items dimmed (`opacity-60`), icons use `icon` class, artist names use MusicBrainz `joinphrase`, charts use CSS Grid (not SVG). ## Routes / Navigation -- **Three routes per resource with show modals:** `:show`, `:edit` (at `/show/edit`), `:add_*` (at `/show/add-*`). -- **Modals close via `JS.patch`** back to the base route. -- **Search state in URL query params** via `push_patch`. -- **Filter empty params from URLs:** `Enum.filter(fn {_, v} -> v not in ["", nil] end)`. -- **Conditional links based on `purchased_at`:** determines `/collection/` vs `/wishlist/` paths. -- **`/dev/` namespace is for developer tooling only** (LiveDashboard, Oban Web, ErrorTracker). User-facing admin routes belong in the main authenticated `live_session`. +→ See `.claude/skills/ui-framework/SKILL.md` for full route patterns. + +Key rules: three routes per resource with show modals, modals close via `JS.patch`, search state in URL query params, empty params filtered from URLs, conditional links based on `purchased_at`, `/dev/` namespace for developer tooling only. ## Database -- **SQLite JSON patterns:** `json_each()` and `json_extract()` via `fragment` for JSON column queries. Expression-based indexes on `json_extract` for performance. -- **Match `GROUP BY` expressions to existing expression indexes.** SQLite treats `json_extract(?, '$.path')` and `? ->> '$.path'` as semantically equal but textually distinct. Use `json_extract` in `fragment` when an expression index on `json_extract(...)` already exists, so the GROUP BY can use the index for natural ordering. -- **Force subquery materialization with `limit: -1` to prevent flattening.** When a date-filtered scan feeds an outer `GROUP BY` and SQLite prefers the wrong composite index, wrap the filter in `subquery()` with `limit: -1`. The forced materialization preserves the range-scan index. -- **Correlated scalar subqueries beat `LEFT JOIN` for small-LIMIT enrichment.** When attaching lookups from a large table to a LIMIT'd result set, inline `(SELECT ... WHERE ...)` in the outer SELECT. Cost then scales with the outer LIMIT, not the lookup table size. -- **Materialized views via triggers** (SQLite lacks native materialized views). Use explicit `up`/`down` in migrations for non-reversible DDL. -- **Read-only schemas** for materialized/view tables: `@primary_key false`, no changeset functions, no timestamps. -- **Every `execute` provides both up and down SQL.** Every index has a comment explaining which query it helps. -- **Config-driven constants.** Pagination defaults and similar magic numbers live in `config/config.exs`, read via `Application.compile_env!/2` into module attributes. +→ See `.claude/skills/sqlite-optimization/SKILL.md` for full query patterns, index rules, FTS5 conventions, and migration requirements. + +Key rules: `json_extract()` must match expression index text exactly, subquery materialization via `limit: -1`, correlated scalar subqueries beat `LEFT JOIN` for small-LIMIT enrichment, every migration provides both `up` and `down` SQL, config-driven constants in `config/config.exs`. ## Error Handling +→ See `.claude/skills/error-investigation/SKILL.md` for production error triage, `.claude/skills/external-api-integration/SKILL.md` for ErrorResponse/ErrorHandler patterns, and `.claude/skills/oban-worker/SKILL.md` for worker return values. + - **Toast notifications:** `put_toast/3` (arity 3) in LiveViews, `put_toast!/2` (arity 2) in LiveComponents. `:info` for success, `:error` for failures. -- **User-facing error reasons use `ErrorMessages.friendly_message/1`** — never `inspect(reason)`. Call sites keep their contextual prefix (e.g. `gettext("Error refreshing cover")`) and append `": " <> ErrorMessages.friendly_message(reason)` for the reason part. `Logger.error` calls keep `inspect` for debugging. +- **User-facing error reasons use `ErrorMessages.friendly_message/1`** — never `inspect(reason)`. Call sites keep their contextual prefix and append `": " <> ErrorMessages.friendly_message(reason)`. `Logger.error` calls keep `inspect` for debugging. - **`handle_async` always handles three cases:** `{:ok, {:ok, result}}`, `{:ok, {:error, reason}}`, and `{:exit, reason}`. - **Data cascade on upstream changes:** When artist metadata changes, regenerate dependent record embeddings. -- **Non-actionable errors use `ErrorTracker.Ignorer`** behaviour to filter (e.g., NoRouteError from bot scanners) rather than blocking paths in the endpoint. -- **Muted errors skip notifications.** `ErrorTracker.ErrorNotifier` checks the `muted` flag before sending email notifications. -- **Oban worker return values follow three states:** `:ok` for success; `{:error, reason}` for transient/retryable failures (Oban will retry); `{:cancel, reason}` (not the deprecated `{:discard, reason}`) for permanent termination when the job outcome is non-retryable (e.g., no Wikipedia entry exists). -- **API failures flow through per-API `ErrorResponse` structs.** Each external API has a `.API.ErrorResponse` struct implementing the `MusicLibrary.ErrorResponse` behaviour (`retryable?/1`, `retry_delay_seconds/1`). Workers match app-layer atom-cancel reasons first (e.g. `:no_english_wikipedia`, `:cover_not_available`), then forward the remaining `{:error, _}` to `MusicLibrary.Worker.ErrorHandler.to_oban_result/1`, which emits `{:snooze, seconds}` for transient errors and `{:cancel, reason}` for permanent ones. -- **Non-fatal enrichment failures are best-effort.** Context functions that enrich records (colors, embeddings) use a private `best_effort_*` helper that logs a warning and returns the unchanged struct rather than surfacing `{:error, reason}` to callers. Business-logic failures (e.g., API calls in `populate_genres/1`) still return `{:error, reason}`. +- **Non-fatal enrichment failures are best-effort.** Context functions that enrich records (colors, embeddings) use a private `best_effort_*` helper that logs a warning and returns the unchanged struct. Business-logic failures still return `{:error, reason}`. ## Testing -- **Feature-specific test setup stays in the test modules that need it**, not in shared case templates (DataCase, ConnCase). Only add to shared templates when every test using that template genuinely needs it. -- **`@tag :logged_out`** for public endpoint tests. **`@tag :capture_log`** on tests with expected error log output. -- **Fixture modules** use `System.unique_integer([:positive])` for unique names and call through context functions (not raw `Repo.insert`). -- **Verify outcomes through context modules**, not just UI assertions. Delete tests assert both `refute has_element?` and `assert_raise Ecto.NoResultsError`. -- **`render_hook/3`** for testing JS hook interactions. -- Avoid starting test descriptions with "it". -- **No boilerplate-only tests.** Do not add test files that just verify Phoenix generator output (e.g., error view literal strings). Tests must exercise application behaviour. -- **Assert specific values, not just shape.** Prefer `assert data == expected` or `assert data["name"] == "Steven Wilson"` over `assert data != nil` or `assert {:ok, _} = result`. Wildcard matches (`_`) in assertions are a signal the test is too vague. -- **Worker tests that enqueue jobs must `assert_enqueued`.** `perform_job` returning `{:ok, []}` is not sufficient — verify the expected downstream workers were enqueued with correct args. -- **Do not test the same guard at every call site.** If a shared check (e.g., session key presence) is enforced in one place, test it once. Do not duplicate the same assertion across every function that calls the shared check. -- **Consolidate identical assertions across endpoints.** When multiple routes share the same plug/middleware behaviour (e.g., auth), test it once with a loop or parameterised approach, not N separate identical tests. -- **Error assertions must match the error type.** `assert {:error, _reason}` is too broad — match the specific error struct or atom (e.g., `%Req.TransportError{reason: :timeout}`, `:no_session_key`). -- **Do not test untestable operations.** If a database operation (e.g., VACUUM) cannot run in the test sandbox, do not write a test that asserts the sandbox error message. Delete or skip it. -- **Email tests use `Swoosh.Adapters.Sandbox`**, not `Swoosh.Adapters.Test`. Each test setup calls `SwooshSandbox.checkout()` and `SwooshSandbox.checkin()` on exit. When the mailer is invoked from a separate process (e.g., a GenServer started in the test), call `SwooshSandbox.allow(self(), pid)` to share the sandbox with that process. +→ See `.claude/skills/testing/SKILL.md` for full conventions, fixture modules, and SQLite/Swoosh/Worker test patterns. + +Key rules: feature setup stays in test modules that need it, fixture modules use `System.unique_integer([:positive])`, assert specific values not just shapes, error assertions match specific error types, worker tests must `assert_enqueued`, no boilerplate-only tests. ## Tech Debt / Hygiene