Graham McIntire
62af9991af
L2: Remove 2-second Process.sleep from post_startup — the try/catch already handles transient noproc errors from the TaskSupervisor. L3: Remove Process.sleep from delete_device — in-flight jobs already handle the race condition via verify_polling_assignment_unchanged. The blocking sleep was unnecessary and delayed web requests by 500ms.
241 lines
9 KiB
Markdown
241 lines
9 KiB
Markdown
# 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.
|