# Codebase Findings

## HIGH

### H2. Missing `@impl true` on callback implementations

Multiple `handle_event` and `handle_info` callbacks are missing `@impl true`. The project compiles with `--warnings-as-errors`, so this will cause compilation failures.

**`lib/towerops_web/live/alert_live/index.ex`** — missing `@impl true` on:
- Line 65: `handle_event("resolve", ...)`
- Line 89: `handle_event("toggle_alert", ...)`
- Line 94: `handle_event("toggle_select", ...)`
- Line 105: `handle_event("select_all", ...)`
- Line 110: `handle_event("select_none", ...)`
- Line 114: `handle_event("bulk_acknowledge", ...)`
- Line 135: `handle_event("bulk_resolve", ...)`
- Line 155: `handle_event("toggle_site", ...)`
- Line 194: `handle_info(_msg, socket)` (catch-all)

**`lib/towerops_web/live/dashboard_live.ex:87`** — missing `@impl true` on:
- Line 87: `handle_info({:alert_changed, _org_id}, socket)`

---

## MEDIUM

### M1. Direct `changeset.changes` access instead of `Ecto.Changeset.get_field/2`

**File**: `lib/towerops_web/live/device_live/form.ex:321,352,383`

The project guidelines explicitly require using `Ecto.Changeset.get_field/2` to access changeset fields, but `changeset.changes` is accessed directly:
- Line 321: `build_device_snmpv3_creds(form_data, changeset.changes)`
- Line 352: `Map.get(changes, :snmpv3_auth_password)`
- Line 383: `changes = changeset.changes`

Also in `lib/towerops/admin.ex:241`: `changes: changeset.changes`

---

### M2. N+1 query: 4 separate count queries for alert counts

**File**: `lib/towerops_web/live/alert_live/index.ex:207-211`

```elixir
counts = %{
  all: Alerts.count_organization_alerts(organization_id),
  critical: Alerts.count_organization_alerts(organization_id, "critical"),
  unresolved: Alerts.count_organization_alerts(organization_id, "unresolved"),
  resolved: Alerts.count_organization_alerts(organization_id, "resolved")
}
```

Each call executes a separate `SELECT COUNT(*)`. A single query with `COUNT(*) FILTER(WHERE ...)` or `CASE WHEN` aggregation could return all counts in one roundtrip.

---

### M3. No URL parameter validation in `DashboardLive.handle_params`

**File**: `lib/towerops_web/live/dashboard_live.ex:37`

```elixir
insight_source = params["insight_source"]
```

The `insight_source` param is assigned directly without validation against allowed values (`preseem`, `snmp`, `gaiia`, `system`). While downstream code likely handles invalid values gracefully, this is inconsistent with the project's URL-sync guidelines.

---

### M4. `Ecto.UUID.dump/1` pattern match can crash

**File**: `lib/towerops_web/live/device_live/show.ex:938-939`

```elixir
{:ok, iid} = Ecto.UUID.dump(interface_id)
{:ok, oid} = Ecto.UUID.dump(org_id)
```

If `interface_id` or `org_id` is not a valid UUID (e.g., race condition where device was deleted between access check and this call), `dump/1` returns `:error`, causing a `MatchError` crash.

---

### M5. `Repo.query!` in LiveView context

**File**: `lib/towerops_web/live/device_live/show.ex:942`

```elixir
result = Towerops.Repo.query!(...)
```

The `!` variant raises on any database error, crashing the LiveView process. A non-bang variant with proper error handling would be more resilient.

---

### M6. Smoke tests use wrong query parameter names

**File**: `test/towerops_web/live/smoke_test.exs:174,187`

```elixir
test "GET /alerts with query filter", %{conn: conn} do
  assert {:ok, _view, _html} = live(conn, ~p"/alerts?severity=critical")
end
```

The test uses `severity=critical` but `AlertLive.Index.handle_params/3` reads `filter`, not `severity`. Similarly, line 187 uses `?status=resolved` but the handler reads `filter=resolved`. Tests pass because `live/2` just mounts, but the params are silently ignored.

---

### M7. Double query in token deletion

**File**: `lib/towerops/accounts.ex:389-390`

```elixir
tokens_to_expire = Repo.all(from t in UserToken, where: t.user_id == ^user.id)
Repo.delete_all(from(t in UserToken, where: t.id in ^Enum.map(tokens_to_expire, & &1.id)))
```

This SELECTs all tokens just to extract IDs, then DELETEs by those IDs. A single `Repo.delete_all` with the user_id condition suffices.

---

### M8. N+1 risk: `membership.user` without preload

**File**: `lib/towerops/billing/billing_notifier.ex:52`

```elixir
memberships |> Enum.map(fn m -> m.user.email end)
```

If `membership` records are not preloaded with `:user`, each access triggers a separate query.

Also in `lib/towerops_web/controllers/api/v1/members_controller.ex:96` and `lib/towerops_web/graphql/types/member.ex:15`.

---

### M9. Process dictionary for cross-component state

**Files**: 
- `lib/towerops_web/user_auth.ex:611,1085` — `Process.put(:unresolved_alert_count, ...)`
- `lib/towerops_web/components/layouts.ex:124` — `Process.get(:unresolved_alert_count, 0)`

The `:unresolved_alert_count` is stored in the process dictionary and read from a layout component. The project guidelines state "Using the process dictionary is typically a sign of unidiomatic code." A socket assign would be cleaner and less fragile.

---

### M10. Many `list_*` functions lack limits

At least 18 `list_*` functions across context modules return `Repo.all` results without any limit, risking unbounded result sets:

- `lib/towerops/reports.ex:19` — `list_reports/1`
- `lib/towerops/sites.ex` — `list_sites_with_coordinates/0`, `list_organization_sites/1`, `list_root_sites/1`
- `lib/towerops/devices.ex` — `list_site_devices/1`, `list_monitored_devices/0`, `list_snmp_enabled_devices/0`, `list_mikrotik_devices_with_api/0`
- `lib/towerops/agents.ex` — `list_all_agent_tokens/0`, `list_cloud_pollers/0`, `list_updatable_agents/0`, `list_online_cloud_pollers/0`, `list_cloud_polled_devices/0`
- `lib/towerops/organizations.ex` — `list_organization_ids/0`, `list_user_organizations/1`, `list_organizations_with_active_subscriptions/0`
- `lib/towerops/monitoring.ex` — `list_checks/2`, `list_checks_for_agent/2`

---

### M11. `DashboardLive` never unsubscribes from PubSub

**File**: `lib/towerops_web/live/dashboard_live.ex:18-22`

`DashboardLive` subscribes to PubSub topics in `mount` but has no cleanup. Phoenix auto-cleans subscriptions on process termination, but if the LiveView is reused across navigations within the same process (LiveView's default behavior), subscriptions accumulate.

---

### M12. Missing `@doc false` or unnecessary `@doc false` on public functions

**File**: `lib/towerops_web/live/alert_live/index.ex`

Functions like `filter_alerts/2`, `sort_alerts/3`, `severity_weight/1`, `severity_color/1`, `age_severity_color/1`, `format_age_minutes/1`, `format_number/1`, `duration_text/1`, `format_duration_minutes/1`, `get_subscriber_count/2`, `get_site_subscriber_count/1` are marked `@doc false` but are used only within the module. These should be `defp` instead of `def`.

---

## LOW

### L1. `then/2` used for socket rebind instead of explicit assignment

**File**: `lib/towerops_web/live/schedule_live/index.ex:188`

```elixir
|> then(&{:noreply, &1})
```

This is a style issue — explicit assignment would be clearer.

---

### L2. `Scope.for_user(nil)` returns `nil`

**File**: `lib/towerops/accounts/scope.ex:39`

```elixir
def for_user(nil), do: nil
```

This means `socket.assigns.current_scope` can be `nil` for unauthenticated users. Most callers handle this with `&&` guards, but it's a fragile pattern — one nil access on `.user` and the LiveView crashes.

---

### L5. `favicon_status/2` in DashboardLive unused argument
### L5. `favicon_status/2` in DashboardLive unused argument

**File**: `lib/towerops_web/live/dashboard_live.ex:267`

```elixir
defp favicon_status(devices_down, _alert_count) when devices_down > 0, do: "red"
```

The second argument `_alert_count` is unused in all but the middle clause, suggesting the function could be simplified.

---

## INFO

### I1. 55 Oban worker modules — consider organizing

**Pattern**: The workers directory has 55 modules. Some have grown large (`discovery_worker.ex` at 522 lines). Consider splitting into subdirectories by domain (monitoring, sync, maintenance, etc.).

### I2. Heavy dashboard data load on every navigation

**File**: `lib/towerops_web/live/dashboard_live.ex:109-187`

`load_dashboard_data/2` fetches alerts, devices, sites, subscribers, impact data, incidents, activity feed, insights, config changes, and a setup checklist on every `handle_params`. For heavily used routes, consider caching or lazy loading some sections.

### I3. SNMP discovery module is very large

**File**: `lib/towerops/snmp/discovery.ex` — 1745+ lines

This single module is the largest in the codebase. Per the project guidelines on module complexity, it should be broken into focused sub-modules.

### I4. `device_live/show.ex` is very large

The device show LiveView is likely the largest LiveView in the codebase. Consider extracting helper modules for chart builders, data loaders, and formatters (some already exist in `live/device_live/helpers/`).

### I5. `.dialyzer_ignore.exs` is clean

Only dependency and vendored-code suppressions — no app-level warnings are suppressed. Good practice.

### I6. No bare `rescue _ ->` catch-all blocks

The codebase properly avoids silent error swallowing. Good practice.