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(() => {