From 73c88f8211a1b4ca600be40cf14e3734fa937881 Mon Sep 17 00:00:00 2001 From: Marko Djordjevic Date: Wed, 18 Feb 2026 15:15:24 +0100 Subject: [PATCH] fix: resolve stale closures in CandleChart click handler Convert drawingState, selectedLineId, dragState, and annotations to refs that stay in sync with state via useEffect. The chart click handler (subscribeClick) captured stale closure values for these variables. Now reads from refs (e.g. drawingStateRef.current) so the handler always sees the latest state, and removed the stale state variables from the useEffect dependency array. Co-Authored-By: Claude Sonnet 4.6 --- openspec/changes/code-review-fix/tasks.md | 2 +- src/components/CandleChart.tsx | 55 ++++++++++++++--------- 2 files changed, 35 insertions(+), 22 deletions(-) diff --git a/openspec/changes/code-review-fix/tasks.md b/openspec/changes/code-review-fix/tasks.md index 6421db8..cd35588 100644 --- a/openspec/changes/code-review-fix/tasks.md +++ b/openspec/changes/code-review-fix/tasks.md @@ -65,7 +65,7 @@ - [x] 7.1 `[opus]` Fix stale closure in `fetchPredictions`: extract `modelInfo` into a `useRef` that stays in sync with state, use ref in `generateCacheKey` (`src/app/page.tsx:489-552`) - [x] 7.2 `[opus]` Add AbortController to `fetchPredictions` and `handleFetchBatchPredictions`: store controller in ref, abort previous on new request, discard stale responses (`src/app/page.tsx:494-663`) -- [ ] 7.3 `[opus]` Fix stale closure in CandleChart click handler: convert `drawingState`, `selectedLineId`, `dragState`, `annotations` to refs, update refs alongside setState, read refs in handler (`src/components/CandleChart.tsx:572-971`) +- [x] 7.3 `[opus]` Fix stale closure in CandleChart click handler: convert `drawingState`, `selectedLineId`, `dragState`, `annotations` to refs, update refs alongside setState, read refs in handler (`src/components/CandleChart.tsx:572-971`) - [ ] 7.4 `[opus]` Fix stale closure in SpanAnnotationManager keyboard handler: wrap `handleDeleteSpan` in `useCallback`, use ref for `selectedSpanId` (`src/components/SpanAnnotationManager.tsx:461-535`) ## 8. Frontend — Memory Leaks & Performance diff --git a/src/components/CandleChart.tsx b/src/components/CandleChart.tsx index ba3336b..b81a65a 100644 --- a/src/components/CandleChart.tsx +++ b/src/components/CandleChart.tsx @@ -151,6 +151,18 @@ const CandleChart = forwardRef( const [selectedLineId, setSelectedLineId] = useState(null); const [dragState, setDragState] = useState<{ lineId: number; endpoint: 'p1' | 'p2' } | null>(null); + // Refs to avoid stale closures in chart click/mouse handlers + const drawingStateRef = useRef(drawingState); + const selectedLineIdRef = useRef(selectedLineId); + const dragStateRef = useRef<{ lineId: number; endpoint: 'p1' | 'p2' } | null>(dragState); + const annotationsRef = useRef(annotations); + + // Keep refs in sync with state + useEffect(() => { drawingStateRef.current = drawingState; }, [drawingState]); + useEffect(() => { selectedLineIdRef.current = selectedLineId; }, [selectedLineId]); + useEffect(() => { dragStateRef.current = dragState; }, [dragState]); + useEffect(() => { annotationsRef.current = annotations; }, [annotations]); + // Track mounted state to avoid hydration mismatch useEffect(() => { setMounted(true); @@ -584,29 +596,29 @@ const CandleChart = forwardRef( if (time === null || price === null) return; // Check if clicking on a selected line's endpoint handle to start dragging - if (selectedLineId !== null && !activeTool) { - const linePrimitive = linePrimitivesRef.current.get(selectedLineId); + if (selectedLineIdRef.current !== null && !activeTool) { + const linePrimitive = linePrimitivesRef.current.get(selectedLineIdRef.current); if (linePrimitive) { const p1 = linePrimitive.getP1(); const p2 = linePrimitive.getP2(); if (isNearEndpoint(timeCoordinate, priceCoordinate, p1.time, p1.price)) { - setDragState({ lineId: selectedLineId, endpoint: 'p1' }); + setDragState({ lineId: selectedLineIdRef.current!, endpoint: 'p1' }); return; } if (isNearEndpoint(timeCoordinate, priceCoordinate, p2.time, p2.price)) { - setDragState({ lineId: selectedLineId, endpoint: 'p2' }); + setDragState({ lineId: selectedLineIdRef.current!, endpoint: 'p2' }); return; } } } // If currently dragging, complete the drag operation - if (dragState) { - const linePrimitive = linePrimitivesRef.current.get(dragState.lineId); + if (dragStateRef.current) { + const linePrimitive = linePrimitivesRef.current.get(dragStateRef.current.lineId); if (linePrimitive) { - const annotation = annotations.find(a => a.id === dragState.lineId); + const annotation = annotationsRef.current.find(a => a.id === dragStateRef.current!.lineId); if (annotation) { // Persist the updated geometry const p1 = linePrimitive.getP1(); @@ -638,7 +650,7 @@ const CandleChart = forwardRef( // Handle line and rectangle drawing if (activeTool === 'line' || activeTool === 'rectangle') { - if (!drawingState) { + if (!drawingStateRef.current) { // First click: record first point and show preview const firstPoint = { time, price }; setDrawingState({ tool: activeTool, firstPoint }); @@ -678,10 +690,11 @@ const CandleChart = forwardRef( const secondPoint = { time, price }; try { - const timestamp = typeof drawingState.firstPoint.time === 'string' - ? Date.parse(drawingState.firstPoint.time) / 1000 - : (drawingState.firstPoint.time as number); - + const currentDrawing = drawingStateRef.current!; + const timestamp = typeof currentDrawing.firstPoint.time === 'string' + ? Date.parse(currentDrawing.firstPoint.time) / 1000 + : (currentDrawing.firstPoint.time as number); + const response = await fetch('/api/annotations', { method: 'POST', headers: { 'Content-Type': 'application/json' }, @@ -691,10 +704,10 @@ const CandleChart = forwardRef( chart_id: activeChartId, color: selectedColor, geometry: { - startTime: typeof drawingState.firstPoint.time === 'string' - ? Date.parse(drawingState.firstPoint.time) / 1000 - : drawingState.firstPoint.time, - startPrice: drawingState.firstPoint.price, + startTime: typeof currentDrawing.firstPoint.time === 'string' + ? Date.parse(currentDrawing.firstPoint.time) / 1000 + : currentDrawing.firstPoint.time, + startPrice: currentDrawing.firstPoint.price, endTime: typeof secondPoint.time === 'string' ? Date.parse(secondPoint.time) / 1000 : secondPoint.time, @@ -788,7 +801,7 @@ const CandleChart = forwardRef( if (response.ok) { seriesRef.current!.detachPrimitive(deleteLineHit.primitive); linePrimitivesRef.current.delete(deleteLineHit.id); - if (selectedLineId === deleteLineHit.id) { + if (selectedLineIdRef.current === deleteLineHit.id) { setSelectedLineId(null); } await fetchAnnotations(); @@ -838,7 +851,7 @@ const CandleChart = forwardRef( // Find annotation at this timestamp (within tolerance) const tolerance = 60; // 60 seconds tolerance - const annotation = annotations.find( + const annotation = annotationsRef.current.find( (a) => markerTypeNames.includes(a.label_type) && Math.abs(a.timestamp - timestamp) < tolerance @@ -876,7 +889,7 @@ const CandleChart = forwardRef( if (lineHit && activeTool !== 'delete') { // Toggle selection - const newSelectedId = selectedLineId === lineHit.id ? null : lineHit.id; + const newSelectedId = selectedLineIdRef.current === lineHit.id ? null : lineHit.id; setSelectedLineId(newSelectedId); // Update all lines' selection state @@ -955,7 +968,7 @@ const CandleChart = forwardRef( // Find annotation at this timestamp (within tolerance) const tolerance = 60; // 60 seconds tolerance - const annotation = annotations.find( + const annotation = annotationsRef.current.find( (a) => markerTypeNames.includes(a.label_type) && Math.abs(a.timestamp - timestamp) < tolerance @@ -972,7 +985,7 @@ const CandleChart = forwardRef( return () => { chartRef.current?.unsubscribeClick(handleClick); }; - }, [activeTool, candles, annotations, annotationTypes, onAnnotationChange, predictionVisible, predictionSpans, predictionSummary, onPredictionClick, onPredictionDismiss, drawingState, selectedColor]); + }, [activeTool, candles, annotationTypes, onAnnotationChange, predictionVisible, predictionSpans, predictionSummary, onPredictionClick, onPredictionDismiss, selectedColor]); // Handle crosshair move to update preview during drawing and dragging useEffect(() => {