From 9d30eae901462d273148e93db4035be89ea01dd8 Mon Sep 17 00:00:00 2001 From: Jan-Henrik Bruhn Date: Sun, 28 Dec 2025 14:08:08 +0100 Subject: [PATCH] fix: Address Copilot review feedback for performance optimizations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix issues identified in Copilot review: 1. Remove throttling from stage drag cursor updates - cursor now updates immediately on drag start for better UX 2. Accumulate wheel deltaY values during throttle period instead of only processing last event - prevents jerky zoom behavior 3. Remove redundant listening={false} props from child elements (inherited from parent Group) 4. Update file documentation to reflect stage drag cursor update functionality These changes improve both performance and user experience while maintaining code clarity. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- .../PatternCanvas/KonvaComponents.tsx | 18 +----- src/hooks/ui/useCanvasViewport.ts | 63 +++++++++---------- 2 files changed, 31 insertions(+), 50 deletions(-) diff --git a/src/components/PatternCanvas/KonvaComponents.tsx b/src/components/PatternCanvas/KonvaComponents.tsx index b640ce9..2d4c27b 100644 --- a/src/components/PatternCanvas/KonvaComponents.tsx +++ b/src/components/PatternCanvas/KonvaComponents.tsx @@ -53,7 +53,6 @@ export const Grid = memo(({ gridSize, bounds, machineInfo }: GridProps) => { points={points} stroke={gridColor} strokeWidth={1} - listening={false} /> ))} {lines.horizontalLines.map((points, i) => ( @@ -62,7 +61,6 @@ export const Grid = memo(({ gridSize, bounds, machineInfo }: GridProps) => { points={points} stroke={gridColor} strokeWidth={1} - listening={false} /> ))} @@ -76,18 +74,8 @@ export const Origin = memo(() => { return ( - - + + ); }); @@ -114,7 +102,6 @@ export const Hoop = memo(({ machineInfo }: HoopProps) => { stroke={hoopColor} strokeWidth={3} dash={[10, 5]} - listening={false} /> { fontFamily="sans-serif" fontStyle="bold" fill={hoopColor} - listening={false} /> ); diff --git a/src/hooks/ui/useCanvasViewport.ts b/src/hooks/ui/useCanvasViewport.ts index d3677d8..88e2048 100644 --- a/src/hooks/ui/useCanvasViewport.ts +++ b/src/hooks/ui/useCanvasViewport.ts @@ -2,7 +2,7 @@ * useCanvasViewport Hook * * Manages canvas viewport state including zoom, pan, and container size - * Handles wheel zoom and button zoom operations + * Handles wheel zoom, button zoom operations, and stage drag cursor updates */ import { @@ -93,43 +93,45 @@ export function useCanvasViewport({ setStagePos({ x: containerSize.width / 2, y: containerSize.height / 2 }); } - // Wheel zoom handler with RAF throttling + // Wheel zoom handler with RAF throttling and delta accumulation const wheelThrottleRef = useRef(null); - const wheelEventRef = useRef | null>(null); + const accumulatedDeltaRef = useRef(0); + const lastPointerRef = useRef<{ x: number; y: number } | null>(null); + const lastStageRef = useRef(null); const handleWheel = useCallback((e: Konva.KonvaEventObject) => { e.evt.preventDefault(); - // Store the latest event - wheelEventRef.current = e; + const stage = e.target.getStage(); + if (!stage) return; - // Cancel pending throttle if it exists + const pointer = stage.getPointerPosition(); + if (!pointer) return; + + // Accumulate deltaY from all events during throttle period + accumulatedDeltaRef.current += e.evt.deltaY; + lastPointerRef.current = pointer; + lastStageRef.current = stage; + + // Skip if throttle already in progress if (wheelThrottleRef.current !== null) { - return; // Throttle in progress, skip this event + return; } // Schedule update on next animation frame (~16ms) wheelThrottleRef.current = requestAnimationFrame(() => { - const throttledEvent = wheelEventRef.current; - if (!throttledEvent) { - wheelThrottleRef.current = null; - return; - } + const accumulatedDelta = accumulatedDeltaRef.current; + const pointer = lastPointerRef.current; + const stage = lastStageRef.current; - const stage = throttledEvent.target.getStage(); - if (!stage) { - wheelThrottleRef.current = null; - return; - } - - const pointer = stage.getPointerPosition(); - if (!pointer) { + if (!pointer || !stage || accumulatedDelta === 0) { wheelThrottleRef.current = null; + accumulatedDeltaRef.current = 0; return; } const scaleBy = 1.1; - const direction = throttledEvent.evt.deltaY > 0 ? -1 : 1; + const direction = accumulatedDelta > 0 ? -1 : 1; setStageScale((oldScale) => { const newScale = Math.max( @@ -145,8 +147,9 @@ export function useCanvasViewport({ return newScale; }); + // Reset accumulator and throttle wheelThrottleRef.current = null; - wheelEventRef.current = null; + accumulatedDeltaRef.current = 0; }); }, []); @@ -199,19 +202,12 @@ export function useCanvasViewport({ setStagePos({ x: containerSize.width / 2, y: containerSize.height / 2 }); }, [initialScale, containerSize]); - // Stage drag handlers with throttled cursor updates - const lastCursorUpdateRef = useRef(0); - + // Stage drag handlers - cursor updates immediately for better UX const handleStageDragStart = useCallback( (e: Konva.KonvaEventObject) => { - const now = Date.now(); - // Throttle cursor updates to ~60fps (16ms) - if (now - lastCursorUpdateRef.current > 16) { - const stage = e.target.getStage(); - if (stage) { - stage.container().style.cursor = "grabbing"; - } - lastCursorUpdateRef.current = now; + const stage = e.target.getStage(); + if (stage) { + stage.container().style.cursor = "grabbing"; } }, [], @@ -223,7 +219,6 @@ export function useCanvasViewport({ if (stage) { stage.container().style.cursor = "grab"; } - lastCursorUpdateRef.current = 0; }, [], );