Repository navigation
fix(canvas): make demo canvases DPR-aware so they render crisp on Retina - #51
Merged
Merged
Conversation
The interactive canvas demos sized their backing store to fixed logical pixels (e.g. 560x240) and let CSS stretch that bitmap to fill the column. On Retina displays the browser upscaled the low-res bitmap ~2x, so the curves, bars, and text read soft. Fix: size each backing store to logical x devicePixelRatio (capped 2x, the convention pretext.html already uses) and scale the 2D context so all drawing stays in logical coordinates. - drivers.html (planar/dynamic diaphragm): the reported case; also added a resize refit. Aligned the DPR cap to 2x. - knots.html (square vs granny knot): 1 canvas. - dac.html: shared hd() helper applied to all 6 vector canvases (staircase, noise-shaping, R-2R ladder, reconstruction filter x2, multibit-vs-sigma-delta spectra x2, tube transfer/wave/spectrum x3). Canvases that read .width live inside their draw loop now capture logical dims once and thread them through. - horizon.html (drawElementImage demo): has width:100% and no max-width, so it stretches past its 320px native size. Sized to displayed-width x DPR and refit on resize. No change to encoding.html (a putImageData pixel demo that already uses image-rendering:pixelated; the setTransform approach would break it) or pretext.html (already DPR-aware). Verified in the local preview at DPR 2: every backing store doubled, all canvases draw and fill the full extent, no console errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Understanding-first reviewThis is a prompt, not a score or merge gate. Before approving, reconstruct the change:
For docs and dependency changes, a compact answer is enough. For behavior or production changes, include the causal path and one concrete counterexample. The author’s claim card belongs in the PR description; this prompt is for the reviewer’s independent reconstruction. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
aadhar-sh | 1c9cccf | Jul 21 2026, 02:06 PM |
oddharsh
added a commit
that referenced
this pull request
Aug 12, 2026
* fix: close the tag-stripping gaps CodeQL keeps finding All 15 open code-scanning alerts are the same 15 that were triaged and dismissed months ago. They came back because #340 renamed holding/ to www/, lwe-pipeline/ to pipelines/lwe/, and moved build.mjs and contract-tests.mjs into scripts/. CodeQL tracks an alert by file path, so every dismissal whose file moved reset to open. The alerts on files that stayed put, in scripts/lib/html-to-md.mjs and check-page-contracts.mjs, are still dismissed, which is what confirms the cause. So this fixes the 11 where fixing is cheaper than re-dismissing, and leaves 4 that no code change should touch. The security half of every old dismissal still holds: lensMarkdown returns JSON that reaches the DOM through esc(), an excerpt is stored escaped, and contentOf feeds link extraction rather than rendering. What those dismissals conceded was a FIDELITY cost, and that part is real and now fixed. `</script >` is a legal end tag and `--!>` legally closes a comment, so a stripper spelled `</script>` or `-->` hands the body through: script source lands in a markdown render, and minified JS lands in a webmention excerpt a human reads. www/_worker.js/lens.js \s* on the closer, --!> on the comment, and \b so <head> stops swallowing a page that opens with <header> and runs to the next literal </head> www/_worker.js/webmention.js same closers in excerptAround www/_worker.js/webmention-send.js same closers in contentOf pipelines/lwe/generate.mjs replaceAll on the accent's #, and --!> on the console.log marker strip scripts/build.mjs escape the backslash and the rest of the metacharacters before turning * into .* lensStripTags loops, and the comment at it says plainly that one pass of /<[^>]+>/g is already a fixpoint: the match runs from a < to the next >, so a surviving < has no > after it and a second pass cannot match. The loop is for CodeQL, which cannot see that argument, and for whoever narrows the pattern later, where the proof stops holding. Better to write that down than to claim a fix the code does not make. The contract test fails if either closer regex goes back, verified by reverting one. Left open deliberately, because the right resolution is a dismissal: #51 www/garage/horizon.html the unsafe parse IS the demo, setHTMLUnsafe beside setHTML on the visitor's own textarea #48 #49 #50 contract-tests two of these are Array.prototype.includes on a list of extracted citations, so they are not string operations at all, let alone URL checks Suite 240 pass, build, pages:check, typecheck, and 130 routes all green. * fix: end tags carry attributes, and one strip pass can re-form what it removes CodeQL re-scanned the changed lines and found two things the first pass got wrong. Both are real, and the second is the interesting one. An end tag may carry attributes. `</script bar>` closes a script element as surely as `</script>` does, so `<\/script\s*>` was still handing the body through, just for a narrower set of inputs than before. Every closer in lensMarkdown, excerptAround and contentOf is `<\/tag\b[^>]*>` now. Removing a span can RE-FORM the thing being removed out of what is left on either side. `<!-` + `<!--x-->` + `-` collapses to `<!--` the moment the middle goes, so one pass leaves a live comment opener behind. The contract test asserts that collapse directly before asserting the fix, so it demonstrates the bug rather than asserting around it. What explains the split in the alerts is the REPLACEMENT, not the pattern. The webmention strippers replace with a space, which keeps the two sides from touching and makes them immune for free; that is why CodeQL never flagged them for this and flagged lens.js twice. lensMarkdown replaces with "" to preserve markdown spacing, so it pays for that with a fixpoint loop instead. The old lensStripTags is now a general untilStable(s, re, repl) covering all five strips, and the comment at it says which of the two problems each caller has. pipelines/lwe/generate.mjs builds its console.log label by allowlisting the wordy part of the marker rather than denylisting delimiters it has to remember. Output is byte-identical on all four call sites, checked against the old form. Suite 242 pass, build, pages:check, typecheck, 130 routes all green. * fix: keep the end-tag fix, drop the fixpoint loops that fixed nothing The loops in the previous commit were wrong and the measurement is why. Keeping the reasoning in the file so it is not re-argued every time this moves. CodeQL wants a fixpoint loop on each strip in lensMarkdown, because removing a span can re-form what was removed out of what is left either side. That really happens here: one pass of the comment strip over `<p>a<!-<!--x-->-</p>` leaves `<p>a<!--</p>`, since the match starts at the INNER opener. It is unreachable anyway, twice over. The re-formed opener is eaten by the generic `<[^>]+>` strip further down, because `<!--</p>` matches whole. And that strip is itself already a fixpoint: its match runs from a `<` to the next `>`, so any `<` surviving a pass has no `>` after it and a second pass cannot match. The bill for looping all three, measured on one 256KB fixture, node, same V8: 6.29ms against 5.94ms median. Roughly 10% of the CPU-bound half of a scan, on the exact path LENS_PARSE_CAP exists to keep under the 10ms Workers ceiling. Real money for a bug nothing can reach, so the loops are out and the four lens.js multi-character alerts want a dismissal instead. The contract test I wrote for the loops is out with them. It passed with the loops reverted, which made it decoration, and this repo has the scar tissue to know better. The two end-tag tests stay and both fail when their fix is reverted, verified. What survives from that commit is the part that was real. An end tag may carry attributes, so `</script bar>` closes a script element as surely as `</script>` does, and `<\/script\s*>` was still handing the body through. Every closer in lensMarkdown, excerptAround and contentOf is `<\/tag\b[^>]*>` now. Suite 241 pass, build, pages:check, typecheck, 130 routes all green. --------- Co-authored-by: Aadharsh Pannirselvam <19518661+oddharsh@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The interactive canvas demos sized their backing store to fixed logical pixels (e.g. 560x240) and let CSS stretch that bitmap to fill the column. On Retina displays the browser upscaled the low-res bitmap ~2x, so the curves, bars, and text read soft. Reported on the planar/dynamic driver demo (
/lwe/drivers); this PR fixes that one and cross-applies to every other canvas rendered the same way.The fix
Size each backing store to
logical x devicePixelRatio(capped 2x, the conventionpretext.htmlalready uses) and scale the 2D context withsetTransformso all drawing stays in logical coordinates. Same look, just sharp.holding/lwe/drivers.htmlholding/lwe/knots.htmlholding/lwe/dac.htmlhd()helper applied to staircase, noise-shaping, R-2R ladder, reconstruction filter x2, multibit-vs-delta-sigma spectra x2, tube transfer/wave/spectrum x3. Canvases that read.widthlive inside their draw loop now capture logical dims once and thread them through (otherwise they would read the scaled size and draw at 2x off).holding/garage/horizon.htmldrawElementImagedemo. Haswidth:100%and nomax-width, so it stretches past its 320px native size. Sized to displayed-width x DPR and refit on resize.Left alone (already correct)
holding/lwe/encoding.htmlis aputImageDatapixel demo (the canvas pixels are the chroma-subsampling data). It already usesimage-rendering:pixelated; thesetTransformapproach would break it.holding/garage/pretext.htmlis already DPR-aware;pretext.lib.js's canvas is offscreen text measurement only.Verification
Local preview at DPR 2: every backing store doubled (dac's ten canvases went to 1120x360, 1120x400, 552x300, etc.), every canvas samples as drawn and fills the full extent (checked bottom-right and full-width baseline rows, so a missing
setTransformwould have shown up), zero console errors on all three pages, and horizon's resize refit fires without throwing.Reviewer notes
These are authoring sources.
build.mjsdoes not touch garage/lwe HTML, so the inline scripts ship as-is; no build-config change needed.🤖 Generated with Claude Code