diff --git a/CLAUDE.md b/CLAUDE.md index d7b4c91a..9645e65c 100644 --- a/CLAUDE.md +++ b/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 + +<%= for alert <- @alerts do %> +
{alert.message}
+<% end %> + + +
+ <%= for {id, alert} <- @streams.alerts do %> +
{alert.message}
+ <% end %> +
+``` + +### 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 diff --git a/assets/js/app.ts b/assets/js/app.ts index 645c6f79..44907860 100644 --- a/assets/js/app.ts +++ b/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 + } } } diff --git a/assets/js/device_list_reorder.ts b/assets/js/device_list_reorder.ts index 06761ea1..a56d296d 100644 --- a/assets/js/device_list_reorder.ts +++ b/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 } } diff --git a/lib/towerops_web/live/device_live/index.ex b/lib/towerops_web/live/device_live/index.ex index c4e8765c..3efe8b6d 100644 --- a/lib/towerops_web/live/device_live/index.ex +++ b/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, diff --git a/priv/static/changelog.txt b/priv/static/changelog.txt index f8535fb1..c21a76b3 100644 --- a/priv/static/changelog.txt +++ b/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