From 0cd7a34c990daecd21eef50965048f8cf5c7c641 Mon Sep 17 00:00:00 2001 From: Marko Djordjevic Date: Wed, 18 Feb 2026 15:18:48 +0100 Subject: [PATCH] fix: replace useState with useRef for preview primitive to prevent memory leak The preview primitive in SpanAnnotationManager was stored in useState, causing unnecessary re-renders and potential memory leaks since the primitive was not cleaned up on unmount. Changed to useRef, updated all read/write sites, removed from dependency arrays, and added unmount cleanup effect that detaches the primitive from the series. Co-Authored-By: Claude Sonnet 4.6 --- openspec/changes/code-review-fix/tasks.md | 2 +- src/components/SpanAnnotationManager.tsx | 54 ++++++++++++++--------- 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/openspec/changes/code-review-fix/tasks.md b/openspec/changes/code-review-fix/tasks.md index f83bfc1..73ba899 100644 --- a/openspec/changes/code-review-fix/tasks.md +++ b/openspec/changes/code-review-fix/tasks.md @@ -70,7 +70,7 @@ ## 8. Frontend — Memory Leaks & Performance -- [ ] 8.1 `[opus]` Fix SpanAnnotationManager preview primitive memory leak: replace `useState` with `useRef` for preview primitive, add cleanup on unmount (`src/components/SpanAnnotationManager.tsx:265-324`) +- [x] 8.1 `[opus]` Fix SpanAnnotationManager preview primitive memory leak: replace `useState` with `useRef` for preview primitive, add cleanup on unmount (`src/components/SpanAnnotationManager.tsx:265-324`) - [ ] 8.2 `[sonnet]` Use `chart.applyOptions()` for theme changes instead of re-creating chart (`src/components/CandleChart.tsx:333`) - [ ] 8.3 `[sonnet]` Remove `fitContent()` from reconciliation effect, only call on initial load (`src/components/SpanAnnotationManager.tsx:160`) - [ ] 8.4 `[opus]` Implement incremental primitive updates in SpanAnnotationManager: update only selection state on selection change, full reconciliation only on annotation list changes (`src/components/SpanAnnotationManager.tsx:104-161`) diff --git a/src/components/SpanAnnotationManager.tsx b/src/components/SpanAnnotationManager.tsx index 8382da7..91749e2 100644 --- a/src/components/SpanAnnotationManager.tsx +++ b/src/components/SpanAnnotationManager.tsx @@ -69,7 +69,7 @@ export default function SpanAnnotationManager({ const [startCandle, setStartCandle] = useState(null); const [endCandle, setEndCandle] = useState(null); const [cursorCandle, setCursorCandle] = useState(null); - const [previewPrimitive, setPreviewPrimitive] = useState(null); + const previewPrimitiveRef = useRef(null); const [popoverOpen, setPopoverOpen] = useState(false); const [editingSpan, setEditingSpan] = useState(null); const primitivesRef = useRef>(new Map()); @@ -80,6 +80,20 @@ export default function SpanAnnotationManager({ selectedSpanIdRef.current = selectedSpanId; }, [selectedSpanId]); + // Cleanup preview primitive on unmount + useEffect(() => { + return () => { + if (previewPrimitiveRef.current && series) { + try { + series.detachPrimitive(previewPrimitiveRef.current); + } catch { + // series may already be disposed + } + previewPrimitiveRef.current = null; + } + }; + }, [series]); + // Find nearest candle to a timestamp const findNearestCandle = (timestamp: number): Candle | null => { if (candles.length === 0) return null; @@ -171,9 +185,9 @@ export default function SpanAnnotationManager({ if (!chart || !series) return; // Clean up preview if tool changes away from span - if (activeTool !== 'span' && previewPrimitive) { - series.detachPrimitive(previewPrimitive); - setPreviewPrimitive(null); + if (activeTool !== 'span' && previewPrimitiveRef.current) { + series.detachPrimitive(previewPrimitiveRef.current); + previewPrimitiveRef.current = null; setInteractionState('idle'); setStartCandle(null); setEndCandle(null); @@ -222,9 +236,9 @@ export default function SpanAnnotationManager({ setPopoverOpen(true); // Clean up preview primitive - if (previewPrimitive) { - series.detachPrimitive(previewPrimitive); - setPreviewPrimitive(null); + if (previewPrimitiveRef.current) { + series.detachPrimitive(previewPrimitiveRef.current); + previewPrimitiveRef.current = null; } } } else if (activeTool === 'delete') { @@ -266,7 +280,7 @@ export default function SpanAnnotationManager({ return () => { chart.unsubscribeClick(handleClick); }; - }, [chart, series, activeTool, interactionState, candles, previewPrimitive, selectedSpanId, onSelectedSpanChange]); + }, [chart, series, activeTool, interactionState, candles, selectedSpanId, onSelectedSpanChange]); // Handle mouse move for preview useEffect(() => { @@ -308,8 +322,8 @@ export default function SpanAnnotationManager({ }; // Remove old preview primitive - if (previewPrimitive) { - series.detachPrimitive(previewPrimitive); + if (previewPrimitiveRef.current) { + series.detachPrimitive(previewPrimitiveRef.current); } // Create new preview primitive @@ -319,7 +333,7 @@ export default function SpanAnnotationManager({ }); series.attachPrimitive(newPreview); - setPreviewPrimitive(newPreview); + previewPrimitiveRef.current = newPreview; }; chart.subscribeCrosshairMove(handleMouseMove); @@ -327,7 +341,7 @@ export default function SpanAnnotationManager({ return () => { chart.unsubscribeCrosshairMove(handleMouseMove); }; - }, [chart, series, activeTool, interactionState, startCandle, candles, previewPrimitive]); + }, [chart, series, activeTool, interactionState, startCandle, candles]); // Handle Escape key to cancel span selection useEffect(() => { @@ -340,9 +354,9 @@ export default function SpanAnnotationManager({ setEndCandle(null); // Clean up preview - if (previewPrimitive && series) { - series.detachPrimitive(previewPrimitive); - setPreviewPrimitive(null); + if (previewPrimitiveRef.current && series) { + series.detachPrimitive(previewPrimitiveRef.current); + previewPrimitiveRef.current = null; } } } @@ -350,7 +364,7 @@ export default function SpanAnnotationManager({ window.addEventListener('keydown', handleKeyDown); return () => window.removeEventListener('keydown', handleKeyDown); - }, [activeTool, interactionState, previewPrimitive, series]); + }, [activeTool, interactionState, series]); // Handle span deletion const handleDeleteSpan = useCallback(async (spanId: number) => { @@ -522,9 +536,9 @@ export default function SpanAnnotationManager({ onSpanAnnotationsChange(); // Clean up preview and reset state - if (previewPrimitive && series) { - series.detachPrimitive(previewPrimitive); - setPreviewPrimitive(null); + if (previewPrimitiveRef.current && series) { + series.detachPrimitive(previewPrimitiveRef.current); + previewPrimitiveRef.current = null; } setInteractionState('idle'); setStartCandle(null); @@ -539,7 +553,7 @@ export default function SpanAnnotationManager({ window.addEventListener('keydown', handleKeyDown); return () => window.removeEventListener('keydown', handleKeyDown); - }, [handleDeleteSpan, spanAnnotations, activeTool, interactionState, startCandle, cursorCandle, spanLabelTypes, activeChartId, previewPrimitive, series, onSpanAnnotationsChange]); + }, [handleDeleteSpan, spanAnnotations, activeTool, interactionState, startCandle, cursorCandle, spanLabelTypes, activeChartId, series, onSpanAnnotationsChange]); // Handle double-click to edit span useEffect(() => {