graham/towerops
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions
towerops/findings.md
Graham McIntire 62af9991af Fix L2, L3: remove blocking Process.sleep calls 
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.
2026-05-29 16:00:07 -05:00

9 KiB
Raw Blame History

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

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

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

{: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

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

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

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

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

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

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

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.