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>pull/15/head
parent
ad0656b1cc
commit
a06ea17f99
|
|
@ -215,17 +215,19 @@ const ChannelCanvas: React.FC<ChannelCanvasProps> = ({
|
||||||
// Re-draw whenever data or sizing changes
|
// Re-draw whenever data or sizing changes
|
||||||
useLayoutEffect(() => {
|
useLayoutEffect(() => {
|
||||||
const canvas = canvasRef.current;
|
const canvas = canvasRef.current;
|
||||||
const wrap = wrapRef.current;
|
const wrap = wrapRef.current;
|
||||||
if (!canvas || !wrap) return;
|
if (!canvas || !wrap) return;
|
||||||
|
|
||||||
const { width, height } = wrap.getBoundingClientRect();
|
const { width, height } = wrap.getBoundingClientRect();
|
||||||
if (width === 0 || height === 0) return;
|
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;
|
canvas.height = Math.floor(height) * window.devicePixelRatio;
|
||||||
const ctx = canvas.getContext('2d');
|
const ctx = canvas.getContext('2d');
|
||||||
if (ctx) ctx.scale(window.devicePixelRatio, window.devicePixelRatio);
|
if (ctx) ctx.scale(window.devicePixelRatio, window.devicePixelRatio);
|
||||||
drawWaveform(canvas, samples, channel.color, windowEndMs, windowMs);
|
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
|
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||||
}, [samples, channel.color, windowEndMs, windowMs]);
|
}, [samples, channel.color, windowEndMs, windowMs]);
|
||||||
|
|
||||||
|
|
@ -246,21 +248,22 @@ interface RulerCanvasProps {
|
||||||
|
|
||||||
const RulerCanvas: React.FC<RulerCanvasProps> = ({ windowEndMs, windowMs, timeDivMs }) => {
|
const RulerCanvas: React.FC<RulerCanvasProps> = ({ windowEndMs, windowMs, timeDivMs }) => {
|
||||||
const canvasRef = useRef<HTMLCanvasElement>(null);
|
const canvasRef = useRef<HTMLCanvasElement>(null);
|
||||||
const wrapRef = useRef<HTMLDivElement>(null);
|
const wrapRef = useRef<HTMLDivElement>(null);
|
||||||
|
|
||||||
useLayoutEffect(() => {
|
useLayoutEffect(() => {
|
||||||
const canvas = canvasRef.current;
|
const canvas = canvasRef.current;
|
||||||
const wrap = wrapRef.current;
|
const wrap = wrapRef.current;
|
||||||
if (!canvas || !wrap) return;
|
if (!canvas || !wrap) return;
|
||||||
|
|
||||||
const { width, height } = wrap.getBoundingClientRect();
|
const { width, height } = wrap.getBoundingClientRect();
|
||||||
if (width === 0 || height === 0) return;
|
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;
|
canvas.height = Math.floor(height) * window.devicePixelRatio;
|
||||||
const ctx = canvas.getContext('2d');
|
const ctx = canvas.getContext('2d');
|
||||||
if (ctx) ctx.scale(window.devicePixelRatio, window.devicePixelRatio);
|
if (ctx) ctx.scale(window.devicePixelRatio, window.devicePixelRatio);
|
||||||
drawRuler(canvas, windowEndMs, windowMs, timeDivMs);
|
drawRuler(canvas, windowEndMs, windowMs, timeDivMs);
|
||||||
|
// Intentionally exclude stable canvasRef/wrapRef and module-level drawRuler.
|
||||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||||
}, [windowEndMs, windowMs, timeDivMs]);
|
}, [windowEndMs, windowMs, timeDivMs]);
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -103,13 +103,10 @@ export const useOscilloscopeStore = create<OscilloscopeState>((set, get) => ({
|
||||||
const buf = s.samples[channelId];
|
const buf = s.samples[channelId];
|
||||||
if (!buf) return s;
|
if (!buf) return s;
|
||||||
|
|
||||||
let next: OscSample[];
|
// Efficient copy: one allocation instead of two (avoids spread + slice).
|
||||||
if (buf.length >= MAX_SAMPLES) {
|
const next = buf.slice();
|
||||||
// Drop the oldest entry (shift)
|
if (next.length >= MAX_SAMPLES) next.shift(); // drop oldest
|
||||||
next = [...buf.slice(1), { timeMs, state }];
|
next.push({ timeMs, state });
|
||||||
} else {
|
|
||||||
next = [...buf, { timeMs, state }];
|
|
||||||
}
|
|
||||||
return { samples: { ...s.samples, [channelId]: next } };
|
return { samples: { ...s.samples, [channelId]: next } };
|
||||||
});
|
});
|
||||||
},
|
},
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue