Cleanup webgl #2
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update introduces GIMP-compatible import and export functionality for pattern formulas, refactors the rendering pipeline to use persistent WebGL-based workers with OffscreenCanvas for improved performance, and enhances UI interactions. The rendering logic now maintains a worker per canvas, supports incremental updates, and provides explicit worker cleanup. The user interface is updated to include "Download PNG" and "Export GIMP Format" buttons, with new event listeners for import/export actions and improved preview handling via event delegation. The WebGL worker script is extensively rewritten for modularity, resource management, and alignment with CPU-side formula logic, including new uniforms and optimized shader code. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant MainJS
participant WebGLWorker
User->>UI: Clicks "Export GIMP Format"
UI->>MainJS: Trigger exportToGimp()
MainJS->>qbist.js: exportToGimpFormat(mainFormula)
qbist.js-->>MainJS: ArrayBuffer (GIMP format)
MainJS->>UI: Initiate file download
User->>UI: Clicks "Import GIMP Pattern"
UI->>MainJS: Trigger importFromGimp()
MainJS->>qbist.js: importFromGimpFormat(ArrayBuffer)
qbist.js-->>MainJS: info object
MainJS->>MainJS: Update mainFormula, refresh UI
User->>UI: Initiates rendering (e.g., preview or export)
UI->>MainJS: Call QbistRenderer.render(info, options)
MainJS->>WebGLWorker: Post "init" or "update" message with formula and options
WebGLWorker-->>MainJS: Post "rendered" message or ImageBitmap for export
MainJS->>UI: Hide loading overlay (if export)
Possibly related PRs
Poem
✨ Finishing touches🧪 Generate unit tests (beta)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (1)
qbistListeners.js (1)
137-140: 🧹 Nitpick (assertive)Use
Number.parseIntwith explicit radixStatic analysis flagged this and it does remove a subtle foot‑gun where strings beginning with
0are parsed as octal in non‑strict mode.-const outputWidth = parseInt(document.getElementById("outputWidth").value) +const outputWidth = Number.parseInt( + document.getElementById("outputWidth").value, + 10 +)Apply the same change to
outputHeightandoversampling.🧰 Tools
🪛 Biome (1.9.4)
[error] 137-137: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
[error] 138-138: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
[error] 139-139: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
index.html(1 hunks)index.js(3 hunks)qbist.js(1 hunks)qbistListeners.js(4 hunks)workerWebGL.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
qbist.js (1)
qbistListeners.js (1)
buffer(76-76)
🪛 Biome (1.9.4)
index.js
[error] 19-19: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
workerWebGL.js
[error] 7-15: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
qbistListeners.js
[error] 119-119: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
index.js (1)
9-11: StaletransferredCanvasestrackingThe set is initialised and partially referenced (removed in
cleanupWorker) but never populated, so the transfer‑guard you had previously is no longer effective. Either remove the set entirely or reinstate theaddlogic—see comment below.workerWebGL.js (1)
157-174: Validate uniform‑array lengths before uploadIf
optimizeever returns shorter arrays,uniform1ivwill perform partial updates leaving stale data in the shader. A quick assert prevents subtle rendering corruption.const { usedTransFlag, usedRegFlag } = optimize(formula) +if (usedTransFlag.length !== 36 || usedRegFlag.length !== 6) { + console.error("Optimizer returned unexpected flag lengths") + return +}
| <button id="downloadButton" type="submit">Download PNG</button> | ||
| <button id="exportGimpButton" type="button"> | ||
| Export GIMP Format | ||
| </button> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor accessibility tweak: provide an explicit aria-label
Screen‑reader users won’t know that “Export GIMP Format” produces a .qbe file unless it is announced. Consider:
-<button id="exportGimpButton" type="button">
- Export GIMP Format
-</button>
+<button id="exportGimpButton"
+ type="button"
+ aria-label="Export pattern as GIMP .qbe file">
+ Export GIMP Format
+</button>This is purely a nicety—feel free to skip if your target audience is known.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button id="downloadButton" type="submit">Download PNG</button> | |
| <button id="exportGimpButton" type="button"> | |
| Export GIMP Format | |
| </button> | |
| <button id="downloadButton" type="submit">Download PNG</button> | |
| <button id="exportGimpButton" | |
| type="button" | |
| aria-label="Export pattern as GIMP .qbe file"> | |
| Export GIMP Format | |
| </button> |
workerWebGL.js
Outdated
| function createShader(gl, type, source) { | ||
| const shader = gl.createShader(type) | ||
| gl.shaderSource(shader, source) | ||
| gl.compileShader(shader) | ||
| if (!gl.getShaderParameter(shader, gl.COMPILE_STATUS)) { | ||
| console.error("Shader compile error:", gl.getShaderInfoLog(shader)) | ||
| gl.deleteShader(shader) | ||
| return null | ||
| } | ||
| return shader | ||
| } | ||
|
|
||
| function createProgram(gl, vsSource, fsSource) { | ||
| const vertexShader = createShader(gl, gl.VERTEX_SHADER, vsSource) | ||
| const fragmentShader = createShader(gl, gl.FRAGMENT_SHADER, fsSource) | ||
| const program = gl.createProgram() | ||
| gl.attachShader(program, vertexShader) | ||
| gl.attachShader(program, fragmentShader) | ||
| gl.linkProgram(program) | ||
| if (!gl.getProgramParameter(program, gl.LINK_STATUS)) { | ||
| console.error("Program link error:", gl.getProgramInfoLog(program)) | ||
| gl.deleteProgram(program) | ||
| return null | ||
| } | ||
| return program | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Detach & delete shaders immediately after linking to prevent GPU leaks
The program is linked but the two compiled shader objects remain allocated for the lifetime of the context. Explicitly detach & delete them once linking succeeds.
gl.attachShader(program, vertexShader)
gl.attachShader(program, fragmentShader)
gl.linkProgram(program)
if (!gl.getProgramParameter(program, gl.LINK_STATUS)) {
@@
}
+
+ // Shaders no longer needed after linking
+ gl.detachShader(program, vertexShader)
+ gl.detachShader(program, fragmentShader)
+ gl.deleteShader(vertexShader)
+ gl.deleteShader(fragmentShader)
return program
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function createShader(gl, type, source) { | |
| const shader = gl.createShader(type) | |
| gl.shaderSource(shader, source) | |
| gl.compileShader(shader) | |
| if (!gl.getShaderParameter(shader, gl.COMPILE_STATUS)) { | |
| console.error("Shader compile error:", gl.getShaderInfoLog(shader)) | |
| gl.deleteShader(shader) | |
| return null | |
| } | |
| return shader | |
| } | |
| function createProgram(gl, vsSource, fsSource) { | |
| const vertexShader = createShader(gl, gl.VERTEX_SHADER, vsSource) | |
| const fragmentShader = createShader(gl, gl.FRAGMENT_SHADER, fsSource) | |
| const program = gl.createProgram() | |
| gl.attachShader(program, vertexShader) | |
| gl.attachShader(program, fragmentShader) | |
| gl.linkProgram(program) | |
| if (!gl.getProgramParameter(program, gl.LINK_STATUS)) { | |
| console.error("Program link error:", gl.getProgramInfoLog(program)) | |
| gl.deleteProgram(program) | |
| return null | |
| } | |
| return program | |
| } | |
| function createProgram(gl, vsSource, fsSource) { | |
| const vertexShader = createShader(gl, gl.VERTEX_SHADER, vsSource) | |
| const fragmentShader = createShader(gl, gl.FRAGMENT_SHADER, fsSource) | |
| const program = gl.createProgram() | |
| gl.attachShader(program, vertexShader) | |
| gl.attachShader(program, fragmentShader) | |
| gl.linkProgram(program) | |
| if (!gl.getProgramParameter(program, gl.LINK_STATUS)) { | |
| console.error("Program link error:", gl.getProgramInfoLog(program)) | |
| gl.deleteProgram(program) | |
| return null | |
| } | |
| // Shaders no longer needed after linking | |
| gl.detachShader(program, vertexShader) | |
| gl.detachShader(program, fragmentShader) | |
| gl.deleteShader(vertexShader) | |
| gl.deleteShader(fragmentShader) | |
| return program | |
| } |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Deploying qbist with
|
| Latest commit: |
ae75c2b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://08dccd72.qbist.pages.dev |
| Branch Preview URL: | https://cleanup-webgl.qbist.pages.dev |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
workerWebGL.js (3)
2-2: Avoid absolute‑root import to prevent 404 under sub‑paths
"/qbist.js"assumes the site is served from/. Deploying under a sub‑folder (/app/) will break the worker load.-import { optimize } from "/qbist.js" +import { optimize } from "../qbist.js" // or construct the URL dynamically
8-15: Split the chainedletdeclaration for clarity & linter compliance
Biome keeps flagging this block (useSingleVarDeclarator). Breaking it out improves diff readability and avoids accidental re‑assignment bugs.-let uResolutionLoc, - uTimeLoc, - uTransformSequenceLoc, - uSourceLoc, - uControlLoc, - uDestLoc, - uUsedTransFlagLoc, - uUsedRegFlagLoc +let uResolutionLoc = null +let uTimeLoc = null +let uTransformSequenceLoc = null +let uSourceLoc = null +let uControlLoc = null +let uDestLoc = null +let uUsedTransFlagLoc = null +let uUsedRegFlagLoc = null🧰 Tools
🪛 Biome (1.9.4)
[error] 7-15: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
81-83: Nit: swapsteparguments for self‑documenting intent
step(vec3(0.0), diff)reads “add 1 if diff < 0”, matching the comment better than the current reversed order.- r[dr] = diff + step(diff, vec3(0.0)); + r[dr] = diff + step(vec3(0.0), diff); // add 1 when component is negativeindex.js (4)
13-20: Terminate the worker instead of just deleting the reference
delete canvas.workerremoves the property but the thread (and its GPU context) keeps running, leaking memory over time.- canvas.worker.postMessage({ type: "cleanup" }) - // Remove from transferred set - delete canvas.worker + try { + canvas.worker.postMessage({ type: "cleanup" }) + } catch (_) { /* ignore */ } + + canvas.worker.terminate() + canvas.worker = undefined transferredCanvases.delete(canvas)🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
30-39: Promise resolves before the new frame is readyWhen re‑using a persistent worker the function resolves immediately after
postMessage("update"); callers (e.g. export flow) may read stale pixels.Wait for the next
"rendered"message, same as the fresh‑worker path.
1-1: Same absolute import path issue as in the worker-import { createInfo, modifyInfo, optimize } from "/qbist.js" +import { createInfo, modifyInfo, optimize } from "./qbist.js"
52-56:⚠️ Potential issueGuard against double
transferControlToOffscreenCalling
transferControlToOffscreen()twice on the same canvas throws.
Track transferred canvases (you already havetransferredCanvases) and skip the call for a canvas that’s been transferred.-let offscreen = canvas.transferControlToOffscreen() +let offscreen +if (!transferredCanvases.has(canvas)) { + offscreen = canvas.transferControlToOffscreen() + transferredCanvases.add(canvas) +} else { + offscreen = canvas.offscreenCanvas +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
index.js(3 hunks)qbistListeners.js(4 hunks)workerWebGL.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
workerWebGL.js (1)
Learnt from: shanecranor
PR: shanecranor/qbist-javascript#2
File: workerWebGL.js:176-196
Timestamp: 2025-04-20T01:59:52.282Z
Learning: The base64 encoded state parameters in this codebase don't need to handle URL-safe Base64 variants or percent-encoded URLs as they won't be used in these contexts.
🧬 Code Graph Analysis (1)
workerWebGL.js (3)
index.js (2)
mainFormula(101-101)mainFormula(101-101)qbist.js (3)
createInfo(25-39)usedTransFlag(81-81)usedRegFlag(82-82)qbistListeners.js (2)
stateObj(46-46)loadStateFromParam(43-63)
🪛 Biome (1.9.4)
index.js
[error] 18-18: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
qbistListeners.js
[error] 122-122: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
workerWebGL.js
[error] 7-15: Declare variables separately
Unsafe fix: Break out into multiple declarations
(lint/style/useSingleVarDeclarator)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
workerWebGL.js (1)
45-56:oversamplingis hard‑coded to 2 – parameter from main thread is ignored
drawQbist()forwards anoversamplingargument, but the worker never receives nor uses it; the shader constant#define OVERSAMPLING 2overrides it.Confirm if dynamic oversampling is required (e.g. higher‑quality exports).
If so, pass the value via a uniform or#definestring replace before compilation; otherwise remove the dead parameter from public APIs.index.js (1)
23-24:oversamplingparameter is unused – dead code or missing feature?
drawQbist(canvas, info, oversampling)accepts a value that is never forwarded to the worker, so callers can’t influence quality.Either plumb the value through (
postMessage, uniform) or remove the parameter to avoid confusion.
workerWebGL.js
Outdated
| function createProgram(gl, vsSource, fsSource) { | ||
| const vertexShader = createShader(gl, gl.VERTEX_SHADER, vsSource) | ||
| const fragmentShader = createShader(gl, gl.FRAGMENT_SHADER, fsSource) | ||
| const program = gl.createProgram() | ||
| if (!program) { | ||
| console.error("Failed to create program") | ||
| return null | ||
| } | ||
|
|
||
| function createInfo() { | ||
| const info = { | ||
| transformSequence: [], | ||
| source: [], | ||
| control: [], | ||
| dest: [], | ||
| } | ||
| for (let k = 0; k < MAX_TRANSFORMS; k++) { | ||
| info.transformSequence.push(randomInt(0, NUM_TRANSFORM_TYPES)) | ||
| info.source.push(randomInt(0, NUM_REGISTERS)) | ||
| info.control.push(randomInt(0, NUM_REGISTERS)) | ||
| info.dest.push(randomInt(0, NUM_REGISTERS)) | ||
| } | ||
| return info | ||
| gl.attachShader(program, vertexShader) | ||
| gl.attachShader(program, fragmentShader) | ||
| gl.linkProgram(program) | ||
| if (!gl.getProgramParameter(program, gl.LINK_STATUS)) { | ||
| console.error("Program link error:", gl.getProgramInfoLog(program)) | ||
| gl.deleteProgram(program) | ||
| return null | ||
| } | ||
| return program | ||
| } |
There was a problem hiding this comment.
Null shaders crash attachShader & useProgram – add defensive checks
createShader may return null on compile failure. Attaching / using a null program causes WebGL errors and the worker dies.
function createProgram(gl, vsSource, fsSource) {
const vertexShader = createShader(gl, gl.VERTEX_SHADER, vsSource)
const fragmentShader = createShader(gl, gl.FRAGMENT_SHADER, fsSource)
- const program = gl.createProgram()
- if (!program) {
+ if (!vertexShader || !fragmentShader) return null
+
+ const program = gl.createProgram()
+ if (!program) {
console.error("Failed to create program")
return null
}
@@
if (!gl.getProgramParameter(program, gl.LINK_STATUS)) {
console.error("Program link error:", gl.getProgramInfoLog(program))
gl.deleteProgram(program)
return null
}
+
+ // Shaders no longer needed on GPU after linking
+ gl.detachShader(program, vertexShader)
+ gl.detachShader(program, fragmentShader)
+ gl.deleteShader(vertexShader)
+ gl.deleteShader(fragmentShader)
return program
}After creating the program:
-program = createProgram(gl, vertexShaderSource, fragmentShaderSource)
-gl.useProgram(program)
+program = createProgram(gl, vertexShaderSource, fragmentShaderSource)
+if (!program) return
+gl.useProgram(program)Also applies to: 267-269
| // Use event delegation for preview clicks since canvases get recreated | ||
| document.getElementById("grid").addEventListener("click", (event) => { | ||
| const canvas = event.target.closest(".preview") | ||
| if (canvas) { | ||
| const index = parseInt(canvas.id.replace("preview", "")) | ||
| Object.assign(mainFormula, formulas[index]) | ||
| generateFormulas() | ||
| updateAll() | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use Number.parseInt() to follow modern JS style & avoid global pollution
-const index = parseInt(canvas.id.replace("preview", ""))
+const index = Number.parseInt(canvas.id.replace("preview", ""), 10)The explicit radix also prevents accidental octal parsing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use event delegation for preview clicks since canvases get recreated | |
| document.getElementById("grid").addEventListener("click", (event) => { | |
| const canvas = event.target.closest(".preview") | |
| if (canvas) { | |
| const index = parseInt(canvas.id.replace("preview", "")) | |
| Object.assign(mainFormula, formulas[index]) | |
| generateFormulas() | |
| updateAll() | |
| }) | |
| } | |
| // Use event delegation for preview clicks since canvases get recreated | |
| document.getElementById("grid").addEventListener("click", (event) => { | |
| const canvas = event.target.closest(".preview") | |
| if (canvas) { | |
| - const index = parseInt(canvas.id.replace("preview", "")) | |
| + const index = Number.parseInt(canvas.id.replace("preview", ""), 10) | |
| Object.assign(mainFormula, formulas[index]) | |
| generateFormulas() | |
| updateAll() | |
| } | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 122-122: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
workerWebGL.js (1)
29-37: 🛠️ Refactor suggestionStill leaking GPU resources – detach & delete shaders after linking
Previous feedback pointed this out; the new code still keeps both compiled shaders alive for the lifetime of the context, which slowly consumes GPU memory (especially when workers are re‑created).
gl.linkProgram(program) if (!gl.getProgramParameter(program, gl.LINK_STATUS)) { console.error("Program link error:", gl.getProgramInfoLog(program)) gl.deleteProgram(program) return null } + // Shaders are no longer needed after a successful link + gl.detachShader(program, vertexShader) + gl.detachShader(program, fragmentShader) + gl.deleteShader(vertexShader) + gl.deleteShader(fragmentShader) + return programindex.js (1)
35-45:⚠️ Potential issueWorker not terminated – thread & WebGL context leak
cleanup()only posts a message; it never callsWorker.terminate().
This exact issue was highlighted earlier and remains unresolved.if (this.worker) { this.worker.postMessage({ type: "cleanup" }) + // Give the worker a chance to tidy up, then kill the thread + this.worker.terminate() this.worker = null }
🧹 Nitpick comments (2)
workerWebGL.js (1)
246-254: Busy RAF loop whenkeepAlive === truebutrefreshEveryFrame === falseIf
needsRenderisfalsethe early‑return block still queues anotherrequestAnimationFrame, resulting in an endless render bounce that does nothing but burns a frame every 16 ms.Add the RAF only when you actually intend to draw:
-if (!RendererState.needsRender && !renderMode.refreshEveryFrame) { - if (renderMode.keepAlive) { - requestAnimationFrame((t) => this.render(t)) - } - return -} +if (!RendererState.needsRender && !renderMode.refreshEveryFrame) returnindex.js (1)
1-1: Unused importoptimize– remove to avoid dead code
optimizeis never referenced in this file, but importing it still downloads & executes the module.-import { createInfo, modifyInfo, optimize } from "./qbist.js" +import { createInfo, modifyInfo } from "./qbist.js"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
index.js(2 hunks)qbist.js(1 hunks)workerWebGL.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qbist.js
🧰 Additional context used
🧠 Learnings (1)
workerWebGL.js (1)
Learnt from: shanecranor
PR: shanecranor/qbist-javascript#2
File: workerWebGL.js:176-196
Timestamp: 2025-04-20T01:59:52.282Z
Learning: The base64 encoded state parameters in this codebase don't need to handle URL-safe Base64 variants or percent-encoded URLs as they won't be used in these contexts.
🧬 Code Graph Analysis (1)
index.js (2)
qbistListeners.js (4)
canvas(120-120)outputWidth(140-140)outputHeight(141-141)oversampling(142-142)qbist.js (9)
info(26-31)info(239-244)createInfo(25-39)i(83-83)i(125-125)i(136-136)i(220-220)i(253-253)modifyInfo(42-76)
🪛 Biome (1.9.4)
index.js
[error] 174-174: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
index.js
Outdated
| } catch (err) { | ||
| this.worker.removeEventListener("message", onMessage) | ||
| reject(err) | ||
| } |
There was a problem hiding this comment.
Loading overlay never hides on render failure
When the worker posts an "error" or the promise rejects, the overlay stays visible, blocking the UI.
Add a finally handler (or explicit hide in the catch) to guarantee removal:
} catch (err) {
this.worker.removeEventListener("message", onMessage)
- reject(new Error(e.data.message))
+ loadingOverlay.style.display = "none"
+ reject(new Error(e.data.message))
}Committable suggestion skipped: line range outside the PR's diff.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
workerWebGL.js (2)
130-131: Consider swappingsteparguments for readability
step(edge, x)is semantically "1 if x ≥ edge". Using the variable value asedgeand a constant asxworks, but the intent ("add 1 when diff ≤ 0") isn't obvious at first glance.- vec3 diff = src - ctrl; - r[dr] = diff + step(diff, vec3(0.0)); + vec3 diff = src - ctrl; + r[dr] = diff + step(vec3(0.0), diff); // add 1 if diff is negative
17-40: 🛠️ Refactor suggestionMissing shader cleanup after linking
The program linking is handled well with proper error checks, but you're not detaching and deleting the shader objects after successful linking, which can lead to GPU memory leaks.
if (!gl.getProgramParameter(program, gl.LINK_STATUS)) { console.error("Program link error:", gl.getProgramInfoLog(program)) gl.deleteProgram(program) return null } + // Shaders no longer needed after linking + gl.detachShader(program, vertexShader) + gl.detachShader(program, fragmentShader) + gl.deleteShader(vertexShader) + gl.deleteShader(fragmentShader) return program
🧹 Nitpick comments (1)
QbistExporter.js (1)
51-55: Use optional chaining for safer DOM element accessConsider using optional chaining to check if the exportCanvas exists and has a parentNode, which will make the code more robust and prevent potential errors.
- if (this.exportCanvas && this.exportCanvas.parentNode) { + if (this.exportCanvas?.parentNode) { console.log(`[Canvas Delete] Removed export canvas`) document.body.removeChild(this.exportCanvas) this.exportCanvas = null }🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
QbistExporter.js(1 hunks)QbistRenderer.js(1 hunks)index.js(2 hunks)workerWebGL.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- index.js
🧰 Additional context used
🧠 Learnings (1)
workerWebGL.js (1)
Learnt from: shanecranor
PR: shanecranor/qbist-javascript#2
File: workerWebGL.js:176-196
Timestamp: 2025-04-20T01:59:52.282Z
Learning: The base64 encoded state parameters in this codebase don't need to handle URL-safe Base64 variants or percent-encoded URLs as they won't be used in these contexts.
🧬 Code Graph Analysis (2)
QbistRenderer.js (1)
index.js (3)
canvas(50-50)loadingOverlay(7-7)loadingBar(8-8)
workerWebGL.js (1)
qbist.js (5)
optimize(80-107)usedTransFlag(81-81)usedRegFlag(82-82)info(26-31)info(239-244)
🪛 Biome (1.9.4)
QbistExporter.js
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (19)
workerWebGL.js (12)
2-2: Good update to the import path.Nice job changing from an absolute to a relative import path for the
optimizefunction, addressing a previous review comment that warned about absolute imports failing when deployed under a sub-path.
5-15: Well-implemented shader creation with proper error handling.The
createShaderfunction includes appropriate error checking, logging, and resource cleanup on failure.
55-56: Good addition of usage flag uniforms.The addition of
uUsedTransFlaganduUsedRegFlagallows optimizations by skipping unused transforms and registers in the shader, which should improve performance.
108-113: Effective register initialization optimization.The conditional initialization of registers based on the
uUsedRegFlagis a good optimization, ensuring that only registers actually used by the formula are populated with meaningful values.
116-116: Great optimization with early skip for unused transforms.Adding the
continuestatement for unused transforms is an excellent optimization that will significantly improve shader performance by avoiding unnecessary calculations.
212-219: Good handling of array uniform locations.The use of
${name}[0]for array uniforms addresses the issue with uniform lookup that can fail on some browsers (particularly Safari). This is a robust cross-browser solution.Also applies to: 230-233
240-245: Effective validation of uniform locations.Checking for
nulluniform locations and logging errors will help catch shader compilation or linking issues early in development.
254-271: Well-implemented register and transform optimization.Using the imported
optimizefunction to determine which transforms and registers are actually used and then mapping the boolean arrays to integer arrays (0/1) for the shader is a clean approach.
285-290: Good rendering optimization logic.The conditional rendering based on
needsRenderandrefreshEveryFrameflags prevents unnecessary rendering cycles, which is important for performance.
356-377: Thorough WebGL resource cleanup.The cleanup method properly releases all WebGL resources (program, shaders, VAO, buffers) and resets state, which is essential to prevent memory leaks.
385-389: Proper worker termination with self.close().Including
self.close()after cleanup ensures the worker thread is properly terminated, addressing a previous review comment.
422-430: Robust error handling with worker termination.The try/catch block with error reporting, cleanup, and worker termination ensures that errors don't leave orphaned workers or unhandled exceptions.
QbistExporter.js (2)
1-7: Well-structured exporter class initialization.The class is properly initialized with null references to resources that will be created and cleaned up during the export process.
8-44: Well-implemented image export flow with proper resource management.The
exportImagemethod has a good structure with:
- Cleanup of any existing resources before starting
- Try/finally block to ensure cleanup happens even on error
- Proper handling of bitmap rendering and download
QbistRenderer.js (5)
1-9: Well-structured renderer class initialization.The class is properly initialized with appropriate properties and logging.
11-24: Good worker setup with error handling.The
_setupWorkermethod properly checks for Worker support and includes error handling for worker initialization failures.
26-36: Effective cleanup with worker termination and state reset.The cleanup method properly terminates the worker, resets state, and includes appropriate logging.
38-103: Well-structured asynchronous render method with proper event handling.The render method:
- Returns a Promise for asynchronous handling
- Sets up message event listeners and cleans them up appropriately
- Handles OffscreenCanvas transfer correctly
- Handles errors properly
105-113: Good implementation of the update method.The update method appropriately handles sending new information to the worker without reinitializing it.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
QbistRenderer.js (1)
40-131: 🛠️ Refactor suggestionDirect references to DOM elements create tight coupling.
The renderer directly accesses DOM elements with specific IDs (loadingOverlay, loadingBar), which creates tight coupling between this class and the HTML structure.
Consider passing UI elements through the options parameter:
- async render(info, options = {}) { + async render(info, options = { + loadingElements: null, + }) { const { keepAlive = false, refreshEveryFrame = false, isExport = false, + loadingElements } = options // Later in the code: - const loadingOverlay = document.getElementById("loadingOverlay") - const loadingBar = document.getElementById("loadingBar") + const loadingOverlay = loadingElements?.overlay + const loadingBar = loadingElements?.bar // And in each reference: - loadingOverlay.style.display = "none" + if (loadingOverlay) loadingOverlay.style.display = "none"This would make the class more reusable across different UI implementations.
workerWebGL.js (2)
128-130: GLSL step function usage is potentially unintuitive.The step function usage in the SHIFTBACK transform is semantically correct but might be confusing.
vec3 diff = src - ctrl; -r[dr] = diff + step(diff, vec3(0.0)); +r[dr] = diff + step(vec3(0.0), -diff); // Add 1 if diff < 0The second version more clearly expresses "add 1 if diff is negative."
17-40: 🛠️ Refactor suggestionMissing shader resource cleanup after program linking.
The shaders should be detached and deleted after successful program linking to prevent memory leaks.
if (!gl.getProgramParameter(program, gl.LINK_STATUS)) { console.error("Program link error:", gl.getProgramInfoLog(program)) gl.deleteProgram(program) return null } +// Shaders no longer needed after linking +gl.detachShader(program, vertexShader) +gl.detachShader(program, fragmentShader) +gl.deleteShader(vertexShader) +gl.deleteShader(fragmentShader) return program
🧹 Nitpick comments (1)
QbistExporter.js (1)
89-99: Use optional chaining for safer property access.Consider using optional chaining for checking parent nodes to simplify the code and make it more robust.
- if (this.exportCanvas && this.exportCanvas.parentNode) { + if (this.exportCanvas?.parentNode) { console.log(`[Canvas Delete] Removed export canvas`) document.body.removeChild(this.exportCanvas) this.exportCanvas = null }🧰 Tools
🪛 Biome (1.9.4)
[error] 94-94: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
QbistExporter.js(1 hunks)QbistRenderer.js(1 hunks)index.html(2 hunks)index.js(2 hunks)webgl2.html(2 hunks)worker.js(1 hunks)workerWebGL.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- worker.js
🚧 Files skipped from review as they are similar to previous changes (2)
- index.html
- index.js
🧰 Additional context used
🧠 Learnings (1)
workerWebGL.js (1)
Learnt from: shanecranor
PR: shanecranor/qbist-javascript#2
File: workerWebGL.js:176-196
Timestamp: 2025-04-20T01:59:52.282Z
Learning: The base64 encoded state parameters in this codebase don't need to handle URL-safe Base64 variants or percent-encoded URLs as they won't be used in these contexts.
🧬 Code Graph Analysis (1)
QbistRenderer.js (1)
index.js (3)
canvas(50-50)loadingOverlay(7-7)loadingBar(8-8)
🪛 Biome (1.9.4)
QbistExporter.js
[error] 94-94: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (25)
webgl2.html (3)
25-29: Good initialization with error fallback.The code properly imports the
createInfofunction from a relative path and initializes a default pattern state, ensuring the app can run even without a URL-provided state.
30-45: Well-implemented state loading with proper error handling.The implementation properly attempts to decode and parse the state from URL parameters with appropriate validation and error handling. The try/catch block ensures the app won't crash on malformed state data.
58-70: Good worker initialization with modern module type.The worker initialization uses the module type and properly transfers ownership of the offscreen canvas with a complete message payload including pattern info and rendering flags.
QbistExporter.js (4)
1-7: Good class structure with proper initialization.The QbistExporter class is well-structured with proper initialization of class members to null.
8-31: Export function with proper resource management.The exportImage method uses try/finally to ensure cleanup happens regardless of success or failure. The method properly creates a temporary canvas, renders the image, and handles different result types.
32-50: Clean bitmap export implementation.The handleBitmapExport method creates temporary DOM elements for exporting, properly cleans them up after use, and includes helpful logging.
52-87: Correct Y-axis flipping for WebGL pixel output.The handlePixelExport method correctly handles WebGL's bottom-left origin by flipping the image vertically, which is necessary for PNG export.
QbistRenderer.js (3)
1-9: Well-structured class initialization with logging.The QbistRenderer class is well-structured with proper initialization and helpful logging.
11-26: Robust worker setup with error handling.The _setupWorker method includes proper error checking and cleanup of existing workers, with a dynamic URL for better cross-browser compatibility.
28-38: Thorough cleanup method with proper resource management.The cleanup method sends a cleanup message to the worker and resets all state, ensuring no resources are leaked.
workerWebGL.js (15)
1-2: Correct import path using relative reference.The code now correctly uses a relative path to import the optimize function, which is better for portability.
4-15: Well-implemented shader creation function with error handling.The createShader function properly creates, compiles, and validates shader objects with appropriate error handling and resource cleanup.
42-66: Well-structured state management with clear separation of concerns.The RendererState object clearly organizes WebGL context, shader program, buffers, uniforms, and rendering mode in a single place.
69-151: Shader code with usage flags for optimization.The shader code is well-structured with uniforms for transform sequence, register usage flags, and proper support for animation. The uUsedTransFlag and uUsedRegFlag arrays are properly used to skip unneeded computations.
154-173: Comprehensive renderer initialization with good options.The Renderer.init method sets up a WebGL2 context with appropriate options for performance and compatibility.
175-186: Program initialization with conditional shader defines.The initProgram method intelligently adds shader defines for animation only when needed.
188-207: Clean geometry setup with VAO for better performance.The initGeometry method properly sets up a Vertex Array Object with buffers and attributes for rendering a fullscreen quad.
209-251: Robust uniform handling with browser compatibility.The initUniforms method properly handles array uniforms with and without [0] suffix for better browser compatibility, with clear error reporting.
253-289: Efficient formula upload with optimization.The uploadFormula method uses the optimize function to determine which transforms and registers are used, properly uploading them to the shader.
291-332: Smart render function with conditional updates.The render method intelligently decides when to update based on needsRender and refreshEveryFrame flags, with proper time handling for animation.
334-370: Comprehensive export handling with fallbacks.The handleExport method tries to use modern ImageBitmap first and falls back to pixel readback if needed.
372-390: Proper pixel buffer export with transfer.The fallbackExport method correctly reads pixels and transfers the buffer for better performance.
392-401: Clean continuous rendering with message feedback.The handleContinuousRender method correctly posts the rendered message and schedules the next frame if needed.
403-425: Thorough cleanup with shader handling.The cleanup method properly cleans up all WebGL resources including attached shaders, with clear logging.
427-478: Well-structured message handling with proper error management.The message event handler properly handles init, update, and cleanup messages with comprehensive error handling that cleans up resources and terminates the worker on fatal errors.
| // Handle window resize | ||
| window.addEventListener("resize", () => { | ||
| resizeCanvas() | ||
| // Update the OffscreenCanvas dimensions. | ||
| offscreen.width = canvas.width | ||
| offscreen.height = canvas.height | ||
| const width = window.innerWidth | ||
| const height = window.innerHeight | ||
| worker.postMessage({ | ||
| type: "resize", | ||
| width: canvas.width, | ||
| height: canvas.height, | ||
| type: "update", | ||
| info: info, | ||
| keepAlive: true, | ||
| refreshEveryFrame: true, | ||
| }) | ||
| // Update resolution uniform | ||
| const gl = canvas.getContext("webgl2") | ||
| if (gl) { | ||
| gl.viewport(0, 0, width, height) | ||
| } | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
The resize handler might not work as intended.
The resize handler attempts to update the WebGL viewport directly, but this won't affect the OffscreenCanvas since its control has been transferred to the worker.
🏁 Script executed:
#!/bin/bash
# Search to verify if the worker handles resize events properly
rg "resize|viewport" --type jsLength of output: 104
🏁 Script executed:
#!/bin/bash
# Display beginning of workerWebGL.js to inspect resize handling
sed -n '1,200p' workerWebGL.jsLength of output: 6069
🏁 Script executed:
#!/bin/bash
# Search for OffscreenCanvas or transferControlToOffscreen usage in webgl2.html
rg -n "OffscreenCanvas|transferControlToOffscreen" --type html webgl2.htmlLength of output: 191
🏁 Script executed:
#!/bin/bash
# Inspect message handling in workerWebGL.js for resize logic
rg -n "onmessage" -A5 workerWebGL.js
rg -n "addEventListener.*message" -A5 workerWebGL.js
rg -n "canvas.width|canvas.height" -A5 workerWebGL.jsLength of output: 1054
🏁 Script executed:
#!/bin/bash
# Inspect how the worker handles 'update' messages
rg -n 'type *=== *"update"' -A20 workerWebGL.jsLength of output: 781
Adjust resize handler to update the OffscreenCanvas size and notify the worker
You’re currently calling canvas.getContext("webgl2") on the main thread—after you’ve transferred its control to a worker—so the viewport update there has no effect on the OffscreenCanvas. Instead, update the OffscreenCanvas dimensions on resize and send the new width/height to the worker so it can call gl.viewport (and reset its resolution uniform).
Locations to update:
- webgl2.html (≈ lines 72–87):
- Set
offscreen.width/heightto the new dimensions. - Include
width/heightin thepostMessagepayload. - Remove the main‐thread
getContext("webgl2")block.
- Set
- workerWebGL.js:
- In the
"update"message handler, readwidth/height, setcanvas.width/.height, then callgl.viewport(0,0,width,height)and update theuResolutionuniform before rendering.
- In the
Suggested diff in webgl2.html:
window.addEventListener("resize", () => {
- const width = window.innerWidth
- const height = window.innerHeight
- worker.postMessage({
- type: "update",
- info,
- keepAlive: true,
- refreshEveryFrame: true,
- })
- // Update resolution uniform on main thread (no effect on OffscreenCanvas)
- const gl = canvas.getContext("webgl2")
- if (gl) {
- gl.viewport(0, 0, width, height)
- }
+ const width = window.innerWidth
+ const height = window.innerHeight
+ // Resize the OffscreenCanvas so the worker sees the new dimensions
+ offscreen.width = width
+ offscreen.height = height
+ worker.postMessage({
+ type: "update",
+ info,
+ keepAlive: true,
+ refreshEveryFrame: true,
+ width,
+ height,
+ })
})And in workerWebGL.js, inside the if (type === "update") block:
if (type === "update") {
+ if (typeof width === "number" && typeof height === "number") {
+ canvas.width = width
+ canvas.height = height
+ const gl = RendererState.gl
+ gl.viewport(0, 0, width, height)
+ // Also upload uResolution uniform here if not done in render()
+ gl.uniform2i(RendererState.uniforms.uResolution, width, height)
+ }
RendererState.formula = info
RendererState.renderMode.refreshEveryFrame = refreshEveryFrame
Renderer.uploadFormula(info)
requestAnimationFrame((time) => Renderer.render(time))
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle window resize | |
| window.addEventListener("resize", () => { | |
| resizeCanvas() | |
| // Update the OffscreenCanvas dimensions. | |
| offscreen.width = canvas.width | |
| offscreen.height = canvas.height | |
| const width = window.innerWidth | |
| const height = window.innerHeight | |
| worker.postMessage({ | |
| type: "resize", | |
| width: canvas.width, | |
| height: canvas.height, | |
| type: "update", | |
| info: info, | |
| keepAlive: true, | |
| refreshEveryFrame: true, | |
| }) | |
| // Update resolution uniform | |
| const gl = canvas.getContext("webgl2") | |
| if (gl) { | |
| gl.viewport(0, 0, width, height) | |
| } | |
| }) | |
| // Handle window resize | |
| window.addEventListener("resize", () => { | |
| const width = window.innerWidth | |
| const height = window.innerHeight | |
| // Resize the OffscreenCanvas so the worker sees the new dimensions | |
| offscreen.width = width | |
| offscreen.height = height | |
| worker.postMessage({ | |
| type: "update", | |
| info, | |
| keepAlive: true, | |
| refreshEveryFrame: true, | |
| width, | |
| height, | |
| }) | |
| }) |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes