memory leak fixing

CLAUDE.md

@@ -720,3 +720,196 @@ end
 - Combine with runtime type checking for complete safety
 - PLT files in `priv/plts/` - don't commit
 - Never suppress valid warnings - fix the root cause
+
+## Memory Leak Prevention
+
+### JavaScript Hook Event Listeners
+
+**Problem**: Event listeners added in LiveView hooks without cleanup cause memory leaks when the hook is destroyed.
+
+**Solution**: Always store event listener references and remove them in `destroyed()`:
+
+```typescript
+const MyHook = {
+  handleClick: null as ((e: Event) => void) | null,
+  handleDrag: null as ((e: DragEvent) => void) | null,
+
+  mounted(this: any) {
+    // Store reference to handler
+    this.handleClick = (e: Event) => {
+      // Handler logic
+    }
+
+    this.el.addEventListener("click", this.handleClick)
+  },
+
+  destroyed(this: any) {
+    // Clean up event listeners
+    if (this.handleClick) {
+      this.el.removeEventListener("click", this.handleClick)
+      this.handleClick = null
+    }
+
+    // Clean up any other state
+    this.someProperty = null
+  }
+}
+```
+
+**Examples Fixed**:
+- `CopyToClipboard` - Added `destroyed()` hook to remove click listener
+- `DeviceListReorder` - Added `destroyed()` to remove dragstart, dragend, dragover, drop listeners
+
+**Chart.js Cleanup** (already correct):
+```typescript
+const SensorChart = {
+  destroyed() {
+    if (this.chart) {
+      this.chart.destroy()  // Prevents canvas memory leaks
+      this.chart = null
+    }
+  }
+}
+```
+
+**Cytoscape Cleanup** (already correct):
+```typescript
+const NetworkMap = {
+  destroyed(this: any) {
+    if (this.cy) {
+      this.cy.destroy()  // Cleans up Cytoscape instance
+      this.cy = null
+    }
+  }
+}
+```
+
+### LiveView Assigns vs Streams
+
+**Problem**: Large collections in assigns are kept in memory for the entire LiveView session.
+
+**When to use streams instead of assigns**:
+- Lists with hundreds or thousands of items
+- Lists that receive frequent updates via PubSub
+- Lists where you only need to add/update/remove individual items
+
+**Example - AlertLive.Index should use streams**:
+
+❌ **Current (memory inefficient)**:
+```elixir
+def mount(_params, _session, socket) do
+  {:ok, socket |> assign(:alerts, load_all_alerts())}
+end
+
+def handle_info({:new_alert, _device_id, _alert}, socket) do
+  # Reloads entire list into memory
+  {:noreply, assign(socket, :alerts, load_all_alerts())}
+end
+```
+
+✅ **Better (use streams)**:
+```elixir
+def mount(_params, _session, socket) do
+  {:ok, socket |> stream(:alerts, load_all_alerts())}
+end
+
+def handle_info({:new_alert, _device_id, alert}, socket) do
+  # Only adds one alert to the stream
+  {:noreply, stream_insert(socket, :alerts, alert, at: 0)}
+end
+
+def handle_info({:alert_resolved, _device_id, alert}, socket) do
+  # Only removes one alert from the stream
+  {:noreply, stream_delete(socket, :alerts, alert)}
+end
+```
+
+Template change:
+```heex
+<!-- Old -->
+<%= for alert <- @alerts do %>
+  <div id={"alert-#{alert.id}"}>{alert.message}</div>
+<% end %>
+
+<!-- New with streams -->
+<div id="alerts" phx-update="stream">
+  <%= for {id, alert} <- @streams.alerts do %>
+    <div id={id}>{alert.message}</div>
+  <% end %>
+</div>
+```
+
+### Pagination for Large Lists
+
+**Problem**: Loading thousands of items into memory for display.
+
+**Solution**: Use pagination (see "Pagination in LiveView" section above).
+
+**When pagination is appropriate**:
+- Discovered devices (already paginated - 20 per page)
+- Audit logs
+- Historical alerts
+- Any list that could grow to hundreds of items
+
+**Example** (from DeviceLive.Index):
+```elixir
+# Don't keep all items in assigns
+all_items = Context.list_all_items()  # Temporary variable
+total_count = length(all_items)
+
+# Only store current page in assigns
+items_page = Enum.slice(all_items, offset, per_page)
+
+socket
+|> assign(:items, items_page)  # Only current page
+|> assign(:pagination, %{...})  # Metadata
+```
+
+### Phoenix.PubSub Subscriptions
+
+**Good news**: Phoenix automatically unsubscribes when a LiveView process terminates - no manual cleanup needed.
+
+**Pattern** (already correct in codebase):
+```elixir
+def mount(_params, _session, socket) do
+  if connected?(socket) do
+    Phoenix.PubSub.subscribe(Towerops.PubSub, "topic")
+  end
+  # No need for terminate/2 callback
+end
+```
+
+### Global Event Listeners (app.ts)
+
+**Safe patterns** (already correct):
+- `document.addEventListener('DOMContentLoaded', ...)` - Only fires once, remains for page lifetime
+- `window.addEventListener("phx:page-loading-start", ...)` - Global events for the application
+- `window.addEventListener("phx:copy", ...)` - Phoenix JS commands
+
+These are acceptable because:
+1. They're registered once when the page loads
+2. They remain active for the entire page lifetime
+3. LiveView patch navigation doesn't reload the page, so no duplicates
+
+### Memory Leak Checklist
+
+When writing new LiveView code or JavaScript hooks:
+
+**LiveView (Elixir)**:
+- [ ] Use `stream/3` for large or frequently-updated lists
+- [ ] Use pagination for lists that could grow large
+- [ ] Avoid storing entire datasets in assigns when only a subset is displayed
+- [ ] PubSub subscriptions clean up automatically (no action needed)
+
+**JavaScript Hooks**:
+- [ ] Every `addEventListener` in `mounted()` has a matching `removeEventListener` in `destroyed()`
+- [ ] Store event handler references (don't use anonymous functions)
+- [ ] Clean up third-party library instances (Chart.js, Cytoscape, etc.) in `destroyed()`
+- [ ] Set hook properties to `null` in `destroyed()` to release references
+
+**Testing for Memory Leaks**:
+- Navigate to a page with a large list
+- Use browser DevTools → Memory → Take heap snapshot
+- Navigate away and back multiple times
+- Take another snapshot
+- Compare - memory should not continuously grow

assets/js/app.ts

@@ -304,8 +304,10 @@ const SensorChart: SensorChartHook = {
 
 // LiveView hook for copying to clipboard
 const CopyToClipboard = {
-  mounted() {
-    this.el.addEventListener("click", (e) => {
+  handleClick: null as ((e: Event) => void) | null,
+
+  mounted(this: any) {
+    this.handleClick = (e: Event) => {
       e.preventDefault()
       const targetId = this.el.dataset.target
       if (!targetId) {
@@ -361,7 +363,16 @@ const CopyToClipboard = {
       } else {
         console.error('No text found to copy')
       }
-    })
+    }
+
+    this.el.addEventListener("click", this.handleClick)
+  },
+
+  destroyed(this: any) {
+    if (this.handleClick) {
+      this.el.removeEventListener("click", this.handleClick)
+      this.handleClick = null
+    }
   }
 }
 

assets/js/device_list_reorder.ts

@@ -13,10 +13,15 @@ interface DragData {
 export const DeviceListReorder = {
   draggedElement: null as HTMLElement | null,
   dragData: null as DragData | null,
+  handleDragStart: null as ((e: DragEvent) => void) | null,
+  handleDragEnd: null as ((e: DragEvent) => void) | null,
+  handleDragOver: null as ((e: DragEvent) => void) | null,
+  handleDrop: null as ((e: DragEvent) => void) | null,
+  dragHandleListeners: [] as Array<{ element: Element; handler: (e: Event) => void }>,
 
   mounted(this: any) {
     // Capture drag start
-    this.el.addEventListener("dragstart", (e: DragEvent) => {
+    this.handleDragStart = (e: DragEvent) => {
       const target = (e.target as HTMLElement).closest("[data-site-id]") as HTMLElement
       if (!target) return
 
@@ -38,10 +43,10 @@ export const DeviceListReorder = {
       }
 
       e.dataTransfer!.effectAllowed = "move"
-    })
+    }
 
     // Clean up on drag end
-    this.el.addEventListener("dragend", (_e: DragEvent) => {
+    this.handleDragEnd = (_e: DragEvent) => {
       if (this.draggedElement) {
         this.draggedElement.classList.remove("opacity-50")
         this.draggedElement = null
@@ -52,10 +57,10 @@ export const DeviceListReorder = {
       this.el.querySelectorAll(".border-blue-500").forEach((el: Element) => {
         el.classList.remove("border-blue-500", "border-t-4", "border-b-4")
       })
-    })
+    }
 
     // Show drop indicator
-    this.el.addEventListener("dragover", (e: DragEvent) => {
+    this.handleDragOver = (e: DragEvent) => {
       e.preventDefault()
       if (!this.dragData) return
 
@@ -72,10 +77,10 @@ export const DeviceListReorder = {
         el.classList.remove("border-blue-500", "border-t-4", "border-b-4")
       })
       target.classList.add("border-blue-500", isBottomHalf ? "border-b-4" : "border-t-4")
-    })
+    }
 
     // Handle drop
-    this.el.addEventListener("drop", (e: DragEvent) => {
+    this.handleDrop = (e: DragEvent) => {
       e.preventDefault()
       if (!this.dragData) return
 
@@ -112,13 +117,52 @@ export const DeviceListReorder = {
       this.el.querySelectorAll(".border-blue-500").forEach((el: Element) => {
         el.classList.remove("border-blue-500", "border-t-4", "border-b-4")
       })
-    })
+    }
+
+    // Add event listeners
+    this.el.addEventListener("dragstart", this.handleDragStart)
+    this.el.addEventListener("dragend", this.handleDragEnd)
+    this.el.addEventListener("dragover", this.handleDragOver)
+    this.el.addEventListener("drop", this.handleDrop)
 
     // Prevent drag handle from triggering row click
+    this.dragHandleListeners = []
     this.el.querySelectorAll(".drag-handle").forEach((handle: Element) => {
-      handle.addEventListener("click", (e: Event) => {
+      const handler = (e: Event) => {
         e.stopPropagation()
-      })
+      }
+      handle.addEventListener("click", handler)
+      this.dragHandleListeners.push({ element: handle, handler })
     })
+  },
+
+  destroyed(this: any) {
+    // Remove main event listeners
+    if (this.handleDragStart) {
+      this.el.removeEventListener("dragstart", this.handleDragStart)
+      this.handleDragStart = null
+    }
+    if (this.handleDragEnd) {
+      this.el.removeEventListener("dragend", this.handleDragEnd)
+      this.handleDragEnd = null
+    }
+    if (this.handleDragOver) {
+      this.el.removeEventListener("dragover", this.handleDragOver)
+      this.handleDragOver = null
+    }
+    if (this.handleDrop) {
+      this.el.removeEventListener("drop", this.handleDrop)
+      this.handleDrop = null
+    }
+
+    // Remove drag handle listeners
+    this.dragHandleListeners.forEach(({ element, handler }) => {
+      element.removeEventListener("click", handler)
+    })
+    this.dragHandleListeners = []
+
+    // Clean up state
+    this.draggedElement = null
+    this.dragData = null
   }
 }

lib/towerops_web/live/device_live/index.ex

@@ -43,6 +43,8 @@ defmodule ToweropsWeb.DeviceLive.Index do
         page = params |> Map.get("page", "1") |> String.to_integer()
         per_page = 20
 
+        # Load all discovered devices - this is temporarily kept in memory for slicing
+        # For very large datasets, consider database-level pagination
         all_discovered = Snmp.list_discovered_devices_for_organization(organization.id)
         total_count = length(all_discovered)
         total_pages = ceil(total_count / per_page)
@@ -51,11 +53,13 @@ defmodule ToweropsWeb.DeviceLive.Index do
         page = max(1, min(page, max(1, total_pages)))
 
         offset = (page - 1) * per_page
+        # Only keep the current page in assigns - don't store all_discovered
         discovered_devices = Enum.slice(all_discovered, offset, per_page)
 
         socket
         |> assign(:active_tab, "discovered")
-        |> assign(:discovered_devices, discovered_devices)
+        # Use temporary assign - will be cleared after render
+        |> assign_new(:discovered_devices, fn -> discovered_devices end)
         |> assign(:pagination, %{
           page: page,
           per_page: per_page,

priv/static/changelog.txt

@@ -5,6 +5,7 @@ Devices Working
 2026-01-31
 * Small ui tweaks
 * Backend refactoring
+* Patched some potential memory leaks
 
 2026-01-30
 * Completely overhaul poller agent to not track any state