Skip to content

Commit 9bcc0b6

Browse files
sauyonclaude
andcommitted
chore(renderer): address gemini review on PR #24
Local gemini-review findings (off-PR): - Extract Op/RecordingCtx/makeCtx into lib/canvas-recorder.ts; both box-drawing.test.ts and powerline.test.ts now consume the shared helper, dropping ~250 lines of duplicated mock-context scaffolding. - Single `as unknown as` cast lives once inside makeRecordingCtx; test files import a value already typed as CanvasRenderingContext2D and pass it straight to draw functions, no per-test cast jump. - Remove the closure allocated by withMirror() on every call to fillStadium/innerStrokeStadium/fillBisector — replaced with an inline `if (mirror) { save; ... }` guard around a no-allocation mirrorAroundCellCenter helper that just does the translate/scale. Gemini-bot inline review on PR #24: - demo/bin/render-test.ts:280 calculateDiffPercent now decodes both PNGs via fast-png (already a project dep) and compares RGBA pixel-by-pixel with a ±2/channel tolerance for AA jitter, instead of byte-comparing the compressed PNG buffers. Dimension mismatch reports 100% diff. Fixes the flakiness the bot flagged: PNG metadata or compression-level changes no longer count as visual regressions. All 69 lib tests still pass; typecheck, fmt, lint clean (the two pre-existing biome warnings in url-detection.test.ts are unchanged). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 5662a31 commit 9bcc0b6

5 files changed

Lines changed: 324 additions & 379 deletions

File tree

demo/bin/render-test.ts

Lines changed: 54 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'fs';
1313
import { dirname, join } from 'path';
1414
import { fileURLToPath } from 'url';
15+
import { decode as decodePng } from 'fast-png';
1516

1617
// Get script directory
1718
const __filename = fileURLToPath(import.meta.url);
@@ -250,33 +251,65 @@ async function main() {
250251
}
251252
}
252253

254+
/** Per-channel tolerance for the pixel diff, in 8-bit units. Tuned to
255+
* absorb the ~1-pixel anti-aliasing jitter we routinely see when the
256+
* same canvas path is re-rasterized across different runs (different
257+
* GPUs, headless-Chrome versions, OSes) while still catching real
258+
* geometric regressions. */
259+
const PIXEL_DIFF_TOLERANCE = 2;
260+
253261
/**
254-
* Calculate approximate difference percentage between two PNG buffers.
255-
* This is a simple comparison - for production you might want pixelmatch.
262+
* Pixel-level visual diff between two PNG buffers.
263+
*
264+
* Both PNGs are decoded to raw RGBA via fast-png (already a project
265+
* dependency — no extra `pixelmatch`/`pngjs` install needed), then
266+
* compared pixel-by-pixel. A pixel counts as different if any RGBA
267+
* channel differs by more than PIXEL_DIFF_TOLERANCE. The percentage
268+
* returned is `(differing pixels / total pixels) * 100`.
269+
*
270+
* The previous implementation compared raw PNG byte buffers — that
271+
* was flaky because PNG metadata, compression-level changes, and
272+
* chunk reordering all produce different byte streams from visually
273+
* identical images, and the byte-count delta has no relationship to
274+
* the visual delta.
275+
*
276+
* A dimension mismatch is reported as 100% diff: the baseline and
277+
* the current render disagree on canvas size, which is by definition
278+
* a regression. (Returning a partial-pixel score would just hide
279+
* the real problem.)
256280
*/
257281
function calculateDiffPercent(buf1: Buffer, buf2: Buffer): number {
258-
// Simple approach: compare decoded pixel data
259-
// For a more accurate comparison, use a library like pixelmatch
260-
261-
// Quick heuristic based on buffer size difference and content
262-
const sizeDiff = Math.abs(buf1.length - buf2.length);
263-
const maxSize = Math.max(buf1.length, buf2.length);
264-
265-
if (sizeDiff > 0) {
266-
// Different sizes means different images
267-
return (sizeDiff / maxSize) * 100;
268-
}
269-
270-
// Compare bytes
271-
let diffBytes = 0;
272-
const minLen = Math.min(buf1.length, buf2.length);
273-
for (let i = 0; i < minLen; i++) {
274-
if (buf1[i] !== buf2[i]) {
275-
diffBytes++;
282+
const a = decodePng(buf1);
283+
const b = decodePng(buf2);
284+
285+
if (a.width !== b.width || a.height !== b.height) return 100;
286+
287+
// fast-png returns Uint8Array for 8-bit channels. Both should have
288+
// the same channel layout (4 = RGBA, 3 = RGB) since they were
289+
// rendered from the same source canvas, but we still index by
290+
// pixel rather than by raw offset to stay robust to a layout
291+
// mismatch in the (highly unusual) case where one side is RGB and
292+
// the other is RGBA.
293+
const channels = Math.min(a.channels, b.channels);
294+
const aStride = a.channels;
295+
const bStride = b.channels;
296+
const totalPixels = a.width * a.height;
297+
298+
let diffPixels = 0;
299+
for (let i = 0; i < totalPixels; i++) {
300+
const ai = i * aStride;
301+
const bi = i * bStride;
302+
for (let c = 0; c < channels; c++) {
303+
if (
304+
Math.abs((a.data[ai + c] as number) - (b.data[bi + c] as number)) > PIXEL_DIFF_TOLERANCE
305+
) {
306+
diffPixels++;
307+
break;
308+
}
276309
}
277310
}
278311

279-
return (diffBytes / maxSize) * 100;
312+
return (diffPixels / totalPixels) * 100;
280313
}
281314

282315
main().catch((e) => {

lib/box-drawing.test.ts

Lines changed: 6 additions & 131 deletions
Original file line numberDiff line numberDiff line change
@@ -14,123 +14,7 @@
1414

1515
import { describe, expect, test } from 'bun:test';
1616
import { drawBoxOrBlock, isBoxOrBlock } from './box-drawing';
17-
18-
type Op =
19-
| { kind: 'fillStyle'; v: string }
20-
| { kind: 'strokeStyle'; v: string }
21-
| { kind: 'lineWidth'; v: number }
22-
| { kind: 'lineCap'; v: string }
23-
| { kind: 'globalAlpha'; v: number }
24-
| { kind: 'fillRect'; x: number; y: number; w: number; h: number }
25-
| { kind: 'save' }
26-
| { kind: 'restore' }
27-
| { kind: 'beginPath' }
28-
| { kind: 'moveTo'; x: number; y: number }
29-
| { kind: 'lineTo'; x: number; y: number }
30-
| {
31-
kind: 'bezierCurveTo';
32-
cp1x: number;
33-
cp1y: number;
34-
cp2x: number;
35-
cp2y: number;
36-
x: number;
37-
y: number;
38-
}
39-
| { kind: 'stroke' }
40-
| { kind: 'translate'; x: number; y: number };
41-
42-
interface RecordingCtx {
43-
ops: Op[];
44-
// Mirrored from CanvasRenderingContext2D for type compat.
45-
fillStyle: string;
46-
strokeStyle: string;
47-
lineWidth: number;
48-
lineCap: string;
49-
globalAlpha: number;
50-
fillRect(x: number, y: number, w: number, h: number): void;
51-
save(): void;
52-
restore(): void;
53-
beginPath(): void;
54-
moveTo(x: number, y: number): void;
55-
lineTo(x: number, y: number): void;
56-
bezierCurveTo(cp1x: number, cp1y: number, cp2x: number, cp2y: number, x: number, y: number): void;
57-
stroke(): void;
58-
translate(x: number, y: number): void;
59-
}
60-
61-
function makeCtx(): RecordingCtx {
62-
const ops: Op[] = [];
63-
let fillStyleBacking = '#000';
64-
let strokeStyleBacking = '#000';
65-
let lineWidthBacking = 1;
66-
let lineCapBacking = 'butt';
67-
let globalAlphaBacking = 1;
68-
return {
69-
ops,
70-
get fillStyle() {
71-
return fillStyleBacking;
72-
},
73-
set fillStyle(v: string) {
74-
fillStyleBacking = v;
75-
ops.push({ kind: 'fillStyle', v });
76-
},
77-
get strokeStyle() {
78-
return strokeStyleBacking;
79-
},
80-
set strokeStyle(v: string) {
81-
strokeStyleBacking = v;
82-
ops.push({ kind: 'strokeStyle', v });
83-
},
84-
get lineWidth() {
85-
return lineWidthBacking;
86-
},
87-
set lineWidth(v: number) {
88-
lineWidthBacking = v;
89-
ops.push({ kind: 'lineWidth', v });
90-
},
91-
get lineCap() {
92-
return lineCapBacking;
93-
},
94-
set lineCap(v: string) {
95-
lineCapBacking = v;
96-
ops.push({ kind: 'lineCap', v });
97-
},
98-
get globalAlpha() {
99-
return globalAlphaBacking;
100-
},
101-
set globalAlpha(v: number) {
102-
globalAlphaBacking = v;
103-
ops.push({ kind: 'globalAlpha', v });
104-
},
105-
fillRect(x, y, w, h) {
106-
ops.push({ kind: 'fillRect', x, y, w, h });
107-
},
108-
save() {
109-
ops.push({ kind: 'save' });
110-
},
111-
restore() {
112-
ops.push({ kind: 'restore' });
113-
},
114-
beginPath() {
115-
ops.push({ kind: 'beginPath' });
116-
},
117-
moveTo(x, y) {
118-
ops.push({ kind: 'moveTo', x, y });
119-
},
120-
lineTo(x, y) {
121-
ops.push({ kind: 'lineTo', x, y });
122-
},
123-
bezierCurveTo(cp1x, cp1y, cp2x, cp2y, x, y) {
124-
ops.push({ kind: 'bezierCurveTo', cp1x, cp1y, cp2x, cp2y, x, y });
125-
},
126-
stroke() {
127-
ops.push({ kind: 'stroke' });
128-
},
129-
translate(x, y) {
130-
ops.push({ kind: 'translate', x, y });
131-
},
132-
};
133-
}
17+
import { type RecordingOp, makeRecordingCtx } from './canvas-recorder';
13418

13519
// Standard cell for tests: 10x20 with a 1px light stroke.
13620
const CW = 10;
@@ -139,21 +23,12 @@ const LT = 1;
13923
const COLOR = '#fff';
14024

14125
function draw(cp: number, lightPx = LT) {
142-
const ctx = makeCtx();
143-
const handled = drawBoxOrBlock(
144-
ctx as unknown as CanvasRenderingContext2D,
145-
cp,
146-
0,
147-
0,
148-
CW,
149-
CH,
150-
COLOR,
151-
lightPx
152-
);
26+
const ctx = makeRecordingCtx();
27+
const handled = drawBoxOrBlock(ctx, cp, 0, 0, CW, CH, COLOR, lightPx);
15328
return { ctx, handled };
15429
}
15530

156-
function rectsOnly(ops: Op[]): { x: number; y: number; w: number; h: number }[] {
31+
function rectsOnly(ops: RecordingOp[]): { x: number; y: number; w: number; h: number }[] {
15732
return ops.flatMap((o) => (o.kind === 'fillRect' ? [{ x: o.x, y: o.y, w: o.w, h: o.h }] : []));
15833
}
15934

@@ -486,9 +361,9 @@ describe('box-drawing', () => {
486361
// back to a LIGHT line regardless of dash weight (vlineMiddle/
487362
// hlineMiddle take .light), so a heavy dash at a tiny cell size
488363
// shouldn't suddenly turn into a heavy bar.
489-
const ctx = makeCtx();
364+
const ctx = makeRecordingCtx();
490365
drawBoxOrBlock(
491-
ctx as unknown as CanvasRenderingContext2D,
366+
ctx,
492367
0x2505, // ━━━ heavy triple dash
493368
0,
494369
0,

0 commit comments

Comments
 (0)