From f12d11334a2f340253ceaf808ceb8dbfbc6d88ad Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 20:33:06 +0000 Subject: [PATCH] fix(composer): center divider strokes and clamp gauge radii in preview canvas Two outstanding CodeRabbit outside-diff findings on this PR: - The divider branch applied its 0.5px centering offset after scaling (`ay * s + 0.5`) instead of before it (`(ay + 0.5) * s`), so at SCALE>1 the stroke bled into the preceding LED row/column instead of straddling its own. - A small imported gauge with a wide lineWidth (e.g. width=1, height=1, lineWidth=3) produced a negative radius, which ctx.ellipse() throws IndexSizeError on, aborting render() for every element still to be drawn. Radii are now clamped to zero. Verified both against current code (neither was fixed by later commits on this branch) and mutation-checked: reverting either fix fails the new test. Co-Authored-By: Claude Opus 5 --- test/test_composer_js_contracts.py | 30 +++++++++++++++++++ .../static/v3/js/composer/composer-canvas.js | 16 ++++++---- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/test/test_composer_js_contracts.py b/test/test_composer_js_contracts.py index eb08e0de..a4525f4c 100644 --- a/test/test_composer_js_contracts.py +++ b/test/test_composer_js_contracts.py @@ -185,3 +185,33 @@ def test_align_clears_the_anchor_and_moves_line_endpoints(): "element resolves to the wrong edge" assert "el.x0" in body and "el.y0" in body, \ "_alignElement no longer moves line endpoints" + + +def test_divider_stroke_is_centered_in_led_pixels(): + """A 0.5 canvas-pixel offset (correct only at SCALE=1) was applied after + scaling instead of before it, so at SCALE>1 the stroke bled into the + preceding LED row/column instead of straddling its own.""" + body = _function_source(CANVAS, "_drawElement") + divider_branch = body[body.index("case 'divider'"):] + divider_branch = divider_branch[:divider_branch.index("case 'pips'")] + assert "ay * s + 0.5" not in divider_branch, \ + "divider still offsets by 0.5 canvas pixels after scaling" + assert "ax * s + 0.5" not in divider_branch, \ + "divider still offsets by 0.5 canvas pixels after scaling" + assert "(ay + 0.5) * s" in divider_branch and "(ax + 0.5) * s" in divider_branch, \ + "divider stroke is not centered within its LED pixel" + + +def test_gauge_radii_are_clamped_to_zero(): + """An imported design can carry a small gauge with a wide lineWidth -- + width=1, height=1, lineWidth=3 sends a negative radius into + ctx.ellipse(), which throws IndexSizeError and aborts render() for every + element still to be drawn, not just the gauge.""" + body = _function_source(CANVAS, "_drawElement") + gauge_branch = body[body.index("case 'gauge'"):] + assert re.search(r"Math\.max\(0,\s*rx\s*-\s*lwPx\s*/\s*2\)", gauge_branch), \ + "gauge x-radius is not clamped to zero" + assert re.search(r"Math\.max\(0,\s*ry\s*-\s*lwPx\s*/\s*2\)", gauge_branch), \ + "gauge y-radius is not clamped to zero" + assert "rx - lwPx / 2" not in re.sub(r"Math\.max\(0,\s*rx\s*-\s*lwPx\s*/\s*2\)", "", gauge_branch), \ + "an unclamped gauge radius is still passed to ctx.ellipse()" diff --git a/web_interface/static/v3/js/composer/composer-canvas.js b/web_interface/static/v3/js/composer/composer-canvas.js index c44940dc..05d6fc8c 100644 --- a/web_interface/static/v3/js/composer/composer-canvas.js +++ b/web_interface/static/v3/js/composer/composer-canvas.js @@ -486,11 +486,11 @@ window.ComposerCanvas = (() => { ctx.lineWidth = s; ctx.beginPath(); if (isH) { - ctx.moveTo(0, ay * s + 0.5); - ctx.lineTo(_canvas.width, ay * s + 0.5); + ctx.moveTo(0, (ay + 0.5) * s); + ctx.lineTo(_canvas.width, (ay + 0.5) * s); } else { - ctx.moveTo(ax * s + 0.5, 0); - ctx.lineTo(ax * s + 0.5, _canvas.height); + ctx.moveTo((ax + 0.5) * s, 0); + ctx.lineTo((ax + 0.5) * s, _canvas.height); } ctx.stroke(); break; @@ -560,13 +560,17 @@ window.ComposerCanvas = (() => { ? Math.max(0, Math.min(100, parseFloat(pvGauge) || 0)) / 100 : Math.max(0, Math.min(100, el.previewPct ?? 65)) / 100; const fillSweep = totalSweep * pct; + // A small imported gauge with a wide lineWidth would otherwise send + // a negative radius into ctx.ellipse(), which throws IndexSizeError + // and aborts the whole render() call. + const arx = Math.max(0, rx - lwPx / 2), ary = Math.max(0, ry - lwPx / 2); // biome-ignore lint/correctness/useQwikValidLexicalScope: not Qwik -- this is an Alpine.js component, and the rule is about Qwik's $() serialization boundary, which does not exist here. const toRad = deg => (deg - 90) * Math.PI / 180; // canvas 0=top, PIL 0=right → offset -90 // Track arc if (el.hasTrack !== false) { ctx.beginPath(); - ctx.ellipse(cx, cy, rx - lwPx / 2, ry - lwPx / 2, 0, toRad(startDeg), toRad(startDeg + totalSweep), false); + ctx.ellipse(cx, cy, arx, ary, 0, toRad(startDeg), toRad(startDeg + totalSweep), false); ctx.strokeStyle = `rgb(${el.trackR ?? 40},${el.trackG ?? 40},${el.trackB ?? 40})`; ctx.lineWidth = lwPx; ctx.stroke(); @@ -574,7 +578,7 @@ window.ComposerCanvas = (() => { // Fill arc if (pct > 0) { ctx.beginPath(); - ctx.ellipse(cx, cy, rx - lwPx / 2, ry - lwPx / 2, 0, toRad(startDeg), toRad(startDeg + fillSweep), false); + ctx.ellipse(cx, cy, arx, ary, 0, toRad(startDeg), toRad(startDeg + fillSweep), false); ctx.strokeStyle = `rgb(${el.r},${el.g},${el.b})`; ctx.lineWidth = lwPx; ctx.stroke();