# Project Conventions

Rules extracted from commit history that are specific to this project and not already covered by `CLAUDE.md` usage rules.

## 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
- If you're working on a GitHub issue, make sure to include the issue ID in the commit message body. Specifies if the commit closes the issue.

## Architecture

- **Context modules own all queries.** LiveViews never query the database directly -- they call context functions.
- **Schemas hold pure accessor/helper functions** on the struct (e.g., `RecordSet.count_by_status/1`). No side effects in schemas.
- **LiveView standard structure:** `mount/3` sets `@current_section`, `handle_params/3` loads data and sets `@page_title` (via pattern-matched private `page_title/2`), `handle_info/2` receives LiveComponent messages.
- **LiveComponents communicate with parent** via `send(self(), {__MODULE__, msg})`.
- **Function components go in domain-specific modules:** `CoreComponents` for generic UI, `RecordComponents` for records, `ScrobbleComponents` for scrobbles, `SearchComponents` for search.
- **External API integrations** follow a three-module pattern: Facade (public API), API (Req HTTP client), Config (NimbleOptions).
- **Behaviours are separate modules** containing only `@callback` definitions. Concrete implementations use `@behaviour` and `@impl true`.
- **Oban workers are thin wrappers** that delegate to context modules. `perform/1` should be minimal.
- **Shared utilities** live at parent namespace level (`MusicLibrary.Batch`, not `MusicLibrary.Records.Batch`).
- **Domain sub-modules group under their context.** Modules strongly related to a context live as sub-modules (e.g., `Chats.StreamProvider`, `Chats.RecordChat`), distinct from shared utilities which live at the parent namespace level.
- **Structured search uses NimbleParsec-based `SearchParser` sub-modules.** Each context that supports `key:value` search syntax has a `SearchParser` sub-module (e.g., `Records.SearchParser`, `ListeningStats.SearchParser`) using NimbleParsec. Parsers support quoted multi-word values, free-text fallback, and normalize results into a map. Tests use doctests on the `parse/1` function.
- **Shared LiveView logic goes in `LiveHelpers.*` modules.** Event handlers, param parsing, and rendering helpers used across 2+ LiveViews are extracted to `LiveHelpers.*` rather than duplicated. When shared helpers need per-caller configuration (routes, labels, context modules), the calling LiveView defines a private config function (e.g., `index_config/0`) and assigns the resulting map in `mount/3`; the helper module reads it from `socket.assigns`.

## Extraction / Refactoring

- Extract when duplicated 3+ times. Identical template markup in 3+ places becomes a function component.
- Delete thin wrapper modules with a single caller -- inline them instead.
- Parameterize the differences when extracting shared logic.
- **When modifying an existing shared component**, refactor all callers to the unified new behavior rather than adding a mode/variant attr. Keep shared components to a single render path.
- Private helpers go at module bottom, public functions first.

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

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

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

## Error Handling

- **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.
- **`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).
- **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}`.

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

## Tech Debt / Hygiene

- **Clean up first, then enforce.** Remove all violations before enabling a lint rule.
- **Reverts are total.** Remove every trace: source, CSS, npm deps, config.
- **Never leak sensitive data in prod.** `show_sensitive_data_on_connection_error: false`.
- **Commits are small and single-purpose.** One logical change per commit.
- **Unused aliases are removed** when their module is no longer referenced. Aliases stay alphabetically sorted.
- **Alias nested modules at the top** rather than referencing them inline (e.g. `alias LastFm.Fixtures.RecentTracks` then `RecentTracks.get()`, not `LastFm.Fixtures.RecentTracks.get()`). Enforced by Credo's `AliasUsage` check.
- **Markdown sanitization via MDEx (ammonia).** Use `Markdown.to_html/1` for user content. Annotate raw output with `# sobelow_skip ["XSS.Raw"]` and a comment explaining the sanitization.
- **Sobelow runs on CI and pre-commit** in skip mode for security analysis.
- **ExSlop checks run via Credo** for code quality: no narrator/boilerplate docs (use `@moduledoc false` instead), no obvious/step/narrator comments, no identity `case`/`with` patterns, no `Repo.all` then filter, no `Enum.map` with inline queries, no unaliased nested module use (nested modules must be aliased at the top). Violations are caught by CI.
- **All modules require `@moduledoc`.** The Credo `ModuleDoc` check is enforced in strict mode.
- **Dialyzer is enabled via `dialyxir` (dev/test only) with the `:no_opaque` flag.** Specs are required and checked. Opaque type checking is disabled to tolerate third-party opaque types (e.g., `Ecto.Changeset`).
- **Validate Docker builder image before updating versions.** When changing `ELIXIR_VERSION`, `OTP_VERSION`, or `DEBIAN_VERSION` in the Dockerfile, run `mise run dev:validate-docker-image` to confirm the generated `hexpm/elixir` tag exists on Docker Hub and supports both `linux/amd64` and `linux/arm64`.

## Reviews & Audits

- **Trace call sites and adjacent layers before flagging.** Surface pattern matching produces false positives. Before claiming a pattern is a problem, follow the actual code paths through every layer that could mitigate it. If you can't show the problem manifests, drop the finding.
- **Project conventions override generic best practices.** "Explicit timeouts", "deterministic test fixtures", "smaller modules", "async queries on mount" are not universal. Where this project diverges, the divergence is deliberate -- read this file first and treat existing patterns as load-bearing.
- **Search closed issues before producing an audit.** Re-raising a finding previously closed as not-planned wastes time. If there is new evidence, link to the prior issue and explain what changed.
- **No tracking-only issues.** Every issue must propose a concrete action. "X is still the same as last audit" or "monitor this module" is not an issue.
- **Use technical terms precisely.** "Circular dependency", "race condition", "N+1 query" have specific definitions. A→C and B→C is not a circular dependency. If the code does not match the definition, find a different framing or drop the finding.
- **Prefer eliminating invalid states at the type level over runtime validation.** When code validates that two options aren't passed together, the better fix is usually to reshape the API so the invalid combination is unrepresentable.

## JavaScript

- **Factory function pattern** for JS hooks when two hooks share logic.
- **Data attributes** (`data-*`) for HTML-to-JS communication. Hooks read `dataset` and `pushEvent` to the server.