You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@claude review

Round-1 follow-ups (b10a0ae) addressing both Gemini items on PR #684:

• Fractional DPR transform — ctx.setTransform now uses canvas.width / width (and canvas.height / height) instead of the raw dpr. On 1.25× / 1.5× displays the buffer dimensions are floored but the transform was raw, so the renderer drew at sub-pixel coordinates the buffer could not represent. Using the actual ratio eliminates the residual blur.
• DPR change listener — added a matchMedia listener that bumps a dprTick state. The canvas effect now depends on it, so dragging the window between displays of different densities or zooming the browser triggers an immediate re-render rather than waiting for the next data refresh.

tsc -b --noEmit and vite build clean.

Claude finished @bootjp's task in 1m 38s —— View job

Review of b10a0aed (round-1 follow-ups)

• Read KeyViz.tsx and analyze the two follow-up changes
• Review fractional DPR transform fix
• Review matchMedia DPR listener
• Post final review

Both Gemini items are correctly resolved. Notes per change:

1. Fractional DPR transform (KeyViz.tsx:138–152)

const dpr = Math.min(window.devicePixelRatio || 1, 4);
canvas.width  = Math.max(1, Math.floor(width  * dpr));
canvas.height = Math.max(1, Math.floor(height * dpr));
...
const sx = width  > 0 ? canvas.width  / width  : dpr;
const sy = height > 0 ? canvas.height / height : dpr;
ctx.setTransform(sx, 0, 0, sy, 0, 0);
ctx.clearRect(0, 0, width, height);

Correct. Using canvas.width / width as the scale factor (rather than raw dpr) means the transform is always derived from the floored buffer dimensions — on 1.25× it's e.g. 151/121 ≈ 1.2479… instead of 1.25, so fillRect coordinates can never land at a fractional buffer pixel.

One nit: the width > 0 guard on lines 149–150 falls back to dpr when width is zero. That path is unreachable in practice (the JSX only renders <canvas> when matrix.rows.length > 0, and a non-empty row set implies width > 0) — no bug, but you could simplify to a single expression if you want to remove dead branches:

ctx.setTransform(canvas.width / width, 0, 0, canvas.height / height, 0, 0);

Minor cosmetic; not blocking.

2. matchMedia DPR listener (KeyViz.tsx:98–108)

const [dprTick, setDprTick] = useState(0);
useEffect(() => {
  if (typeof window === "undefined" || !window.matchMedia) return undefined;
  const mq = window.matchMedia(`(resolution: ${window.devicePixelRatio}dppx)`);
  const onChange = () => setDprTick((t) => t + 1);
  mq.addEventListener("change", onChange);
  return () => mq.removeEventListener("change", onChange);
}, [dprTick]);

Correct. The chain-of-one-shot-listeners pattern is the standard approach (Andrew Huth / various React DPR hooks use the same idiom). Each time dprTick increments:

• the old listener is cleaned up;
• a new (resolution: Xdppx) query is built from the freshly updated window.devicePixelRatio;
• dprTick is in the canvas effect's dep list, so the heatmap immediately re-renders at the new DPR.

No infinite-loop risk: the listener only fires when the resolution crosses the threshold, not on arbitrary re-renders. Unmount/StrictMode double-invoke is handled cleanly by the cleanup return.

Two minor observations:

1. return undefined (line 100) — returning undefined explicitly is equivalent to returning nothing, but return; or just no early return would be more conventional. Not a bug, just style.

2. (resolution: Xdppx) exact-match semantics — this fires whenever the DPR moves away from the registered value (either direction: 2→1.5 or 2→3). That's the desired behaviour. Worth knowing: on Firefox, browser zoom changes devicePixelRatio in 0.1× steps; the listener will fire on every zoom step, which is intentional and correct here.

Overall

Both fixes are solid and directly address the Gemini feedback. The fractional-DPR transform is the higher-impact fix (eliminates sub-pixel blur on half-integer DPRs); the matchMedia listener is a nice UX improvement for multi-display users. The implementation is safe, well-commented, and the dep chains are correct.

Approve with the optional nit on the width > 0 guard.

@claude review

Round-2 cosmetic follow-up (7a77b57): hoisted the empty-matrix early return ahead of setTransform. The live path now collapses to a single ctx.setTransform(canvas.width / width, 0, 0, canvas.height / height, 0, 0) with no width > 0 ternary. The empty-matrix branch resets the transform first (in case the canvas was reused) before clearing — makes the dead-state cleanup explicit.

tsc -b --noEmit and vite build clean.

Claude finished @bootjp's task in 1m 2s —— View job

Review of round-2 follow-up (7a77b57)

• Read trigger comment and understand request
• Read KeyViz.tsx and analyze round-2 changes
• Post review feedback

The round-2 changes are correct and, notably, more than cosmetic — see below.

1. Hoisted empty-matrix guard (KeyViz.tsx:143–149)

if (matrix.rows.length === 0 || matrix.column_unix_ms.length === 0) {
  ctx.setTransform(1, 0, 0, 1, 0, 0);
  ctx.clearRect(0, 0, canvas.width, canvas.height);
  return;
}
ctx.setTransform(canvas.width / width, 0, 0, canvas.height / height, 0, 0);
ctx.clearRect(0, 0, width, height);

This is more than cosmetic. When matrix.column_unix_ms.length === 0, width = 0, so canvas.width / width = Infinity. Round-1 guarded this with a ternary fallback to dpr; round-2 makes the zero case unreachable before the division entirely, which is the cleaner fix. The explicit identity reset (1, 0, 0, 1, 0, 0) + clearRect in physical coordinates is the right idiom for a "wipe a potentially reused canvas to a known state" path.

One minor note on the empty-branch clearRect: it correctly uses (0, 0, canvas.width, canvas.height) — physical pixel coordinates under the identity transform. Contrast with the live path's (0, 0, width, height) under the scaled transform; both flush the same backing region, but having different coordinate systems in the two branches is a small readability snag for anyone who reads these side-by-side without the transform context in mind. Not a bug; just worth knowing.

2. Simplified live path

ctx.setTransform(canvas.width / width, 0, 0, canvas.height / height, 0, 0);

Clean. The ternary is gone because the division-by-zero case is now structurally excluded, not just guarded. No issues.

3. Comments

The inline comments accurately explain the transform choice and the reuse concern. Minor phrasing note: "on the off-chance the canvas was reused" slightly undersells the situation — every time data transitions from populated → empty (or on StrictMode double-invoke), the canvas really is reused and the old DPR transform really is live. "in case the canvas carries a stale DPR transform from the previous render" would be more accurate, but this is purely cosmetic.

Overall

Both structural changes (hoisted guard, removed ternary) are correct and the implementation is clean. The div-by-zero elimination is a genuine correctness improvement on top of the stated cosmetic goal.

Approve.