From a06ea17f9921863842916eb0d41a97422dbdf40b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 15:13:17 +0000 Subject: [PATCH] refactor: address code review feedback on oscilloscope - Improve ring buffer efficiency: one array copy instead of two (slice+shift+push vs slice+spread) - Fix extra whitespace in canvas dimension assignments - Add explanatory comments for eslint-disable-next-line react-hooks/exhaustive-deps Co-authored-by: davidmonterocrespo24 <47928504+davidmonterocrespo24@users.noreply.github.com> --- frontend/src/components/simulator/Oscilloscope.tsx | 13 ++++++++----- frontend/src/store/useOscilloscopeStore.ts | 11 ++++------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/frontend/src/components/simulator/Oscilloscope.tsx b/frontend/src/components/simulator/Oscilloscope.tsx index 4b90e44..9c6bbdd 100644 --- a/frontend/src/components/simulator/Oscilloscope.tsx +++ b/frontend/src/components/simulator/Oscilloscope.tsx @@ -215,17 +215,19 @@ const ChannelCanvas: React.FC = ({ // Re-draw whenever data or sizing changes useLayoutEffect(() => { const canvas = canvasRef.current; - const wrap = wrapRef.current; + const wrap = wrapRef.current; if (!canvas || !wrap) return; const { width, height } = wrap.getBoundingClientRect(); if (width === 0 || height === 0) return; - canvas.width = Math.floor(width) * window.devicePixelRatio; + canvas.width = Math.floor(width) * window.devicePixelRatio; canvas.height = Math.floor(height) * window.devicePixelRatio; const ctx = canvas.getContext('2d'); if (ctx) ctx.scale(window.devicePixelRatio, window.devicePixelRatio); drawWaveform(canvas, samples, channel.color, windowEndMs, windowMs); + // Intentionally exclude canvasRef/wrapRef (stable refs) and the + // module-level drawWaveform function from the dependency array. // eslint-disable-next-line react-hooks/exhaustive-deps }, [samples, channel.color, windowEndMs, windowMs]); @@ -246,21 +248,22 @@ interface RulerCanvasProps { const RulerCanvas: React.FC = ({ windowEndMs, windowMs, timeDivMs }) => { const canvasRef = useRef(null); - const wrapRef = useRef(null); + const wrapRef = useRef(null); useLayoutEffect(() => { const canvas = canvasRef.current; - const wrap = wrapRef.current; + const wrap = wrapRef.current; if (!canvas || !wrap) return; const { width, height } = wrap.getBoundingClientRect(); if (width === 0 || height === 0) return; - canvas.width = Math.floor(width) * window.devicePixelRatio; + canvas.width = Math.floor(width) * window.devicePixelRatio; canvas.height = Math.floor(height) * window.devicePixelRatio; const ctx = canvas.getContext('2d'); if (ctx) ctx.scale(window.devicePixelRatio, window.devicePixelRatio); drawRuler(canvas, windowEndMs, windowMs, timeDivMs); + // Intentionally exclude stable canvasRef/wrapRef and module-level drawRuler. // eslint-disable-next-line react-hooks/exhaustive-deps }, [windowEndMs, windowMs, timeDivMs]); diff --git a/frontend/src/store/useOscilloscopeStore.ts b/frontend/src/store/useOscilloscopeStore.ts index 98a6d4b..9f46d83 100644 --- a/frontend/src/store/useOscilloscopeStore.ts +++ b/frontend/src/store/useOscilloscopeStore.ts @@ -103,13 +103,10 @@ export const useOscilloscopeStore = create((set, get) => ({ const buf = s.samples[channelId]; if (!buf) return s; - let next: OscSample[]; - if (buf.length >= MAX_SAMPLES) { - // Drop the oldest entry (shift) - next = [...buf.slice(1), { timeMs, state }]; - } else { - next = [...buf, { timeMs, state }]; - } + // Efficient copy: one allocation instead of two (avoids spread + slice). + const next = buf.slice(); + if (next.length >= MAX_SAMPLES) next.shift(); // drop oldest + next.push({ timeMs, state }); return { samples: { ...s.samples, [channelId]: next } }; }); },