Add input validation and coercion across API modules#434
Merged
tracygardner merged 6 commits intomainfrom Mar 20, 2026
Merged
Conversation
- shapes.js: validate IDs (non-empty string, ≤100 chars) for all create functions; add sensible defaults and toDim() clamping for dimensions that previously had no defaults (cylinder, capsule, plane); clamp alpha to [0,1]; validate text/size/depth in create3DText - animate.js: clamp duration to a positive finite number in rotateAnim and glideTo; coerce x/y/z to finite; guard easing string before calling flock.BABYLON[easing]() to prevent crash on unknown easing name - sound.js: clamp volume to [0,1] and playbackRate to positive in playSound; validate soundName is a non-empty string; clamp bpm and convert setBPM throw to a warn+resolve so it no longer crashes - events.js: guard handler/callback is a function before registering in onEvent, whenActionEvent and whenKeyEvent - control.js: coerce wait() duration to a non-negative finite ms value; guard waitUntil() conditionFunc is a function before polling - transform.js: add toFinite() helper; coerce x/y/z in positionAt, moveByVector, rotate, rotateTo and scale to finite numbers with sensible defaults so non-finite inputs never reach Babylon.js - scene.js: validate sourceMeshName and cloneId in cloneMesh (non-empty string, ≤100 chars) before proceeding https://claude.ai/code/session_01GFLGzae6RMuSSnEo5ax2vH
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds defensive input validation and numeric sanitization across multiple API modules: coerces numeric inputs, validates string IDs and callback types, applies defaults, and returns early with warnings on invalid inputs. Changes
Sequence Diagram(s)(Skipped — changes are defensive validation and normalization across functions; no new multi-component control flow introduced.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Babylon.js 8.x speculatively dynamic-imports WGSL (WebGPU) shader files even when the app only uses the standard WebGL engine (BABYLON.Engine). Those files don't exist in the installed package, so Vite dev mode emits ERR_FAILED console errors for every shader include requested. Add a Vite plugin that intercepts any module id containing "ShadersWGSL" and returns an empty string export, letting the dynamic imports resolve cleanly. Babylon.js then falls back to its WebGL (GLSL) shaders as normal with no functional change and no more ERR_FAILED noise in the console. https://claude.ai/code/session_01GFLGzae6RMuSSnEo5ax2vH
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
api/shapes.js (1)
251-261: JS destructuring defaults don't protect against explicitnull— use the nullish coalescing pattern instead.JS parameter defaults only apply to
undefined, notnull. While the codebase doesn't currently passnullto these factory methods, usingoptions ?? {}instead of= {}is more defensive and clearer about intent. This pattern ensures the code is robust against future callers or dynamic scenarios wherenullmight be supplied.Also applies to:
Lines 335-345, 417-428, 515-517, 611-611, 693-703🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/shapes.js` around lines 251 - 261, The parameter destructuring in createBox currently uses an = {} default which won't guard against callers passing explicit null; change the signature so you accept an options param and destructure from options ?? {} (e.g., keep createBox(boxId, options) then const { width = 1, height = 1, depth = 1, color = "#9932CC", position = new flock.BABYLON.Vector3(0,0,0), alpha = 1, callback = null } = options ?? {}), and apply the same nullish-coalescing destructuring fix to the other factory functions with the same pattern shown in the diff (the later parameter blocks at the lines referenced).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/animate.js`:
- Around line 238-241: The code currently coerces x/y/z to numbers
unconditionally, which destroys the sentinel string "__ground__level__" used by
glideTo; update the coercion so that x, y, and z are left unchanged if they are
the sentinel string (e.g., value === "__ground__level__"), otherwise coerce to
Number and apply the existing numeric fallback (duration keep existing numeric
check). Locate where duration, x, y, z are normalized (variables named duration,
x, y, z inside glideTo/animate) and change those expressions to first check for
the sentinel string, preserving it, then do Number(...) coercion/fallback; make
the same change for the repeated block at lines 247-250.
- Around line 187-190: The current check uses typeof flock.BABYLON[easing] ===
"function" which can match non-easing constructors and cause setEasingMode on
invalid instances; change the guard to verify the constructor actually extends
Babylon's EasingFunction (e.g., ensure flock.BABYLON[easing] &&
flock.BABYLON[easing].prototype instanceof flock.BABYLON.EasingFunction) or
replace it with a whitelist of known easing constructor names before
instantiating and calling setEasingMode; apply the same fix to the other
occurrence around the 303-308 block so rotateAnimation.setEasingFunction is only
called with valid EasingFunction-derived instances.
In `@api/control.js`:
- Around line 13-15: The computed delay (ms) from duration can exceed Node's
setTimeout ceiling (2,147,483,647 ms) causing a TimeoutOverflowWarning and
unexpected 1ms behavior; update the ms calculation in the wait/delay logic to
clamp to the maximum safe timer value by taking Math.min(the computed
non-negative milliseconds, 2147483647) (keep the existing non-negative check),
and reuse that clamped value wherever ms is used (reference the ms variable and
the wait()/delay code paths that consume it).
In `@api/scene.js`:
- Around line 539-547: cloneMesh currently only validates sourceMeshName and
cloneId but allows a truthy non-function callback that later causes a runtime
error when requestAnimationFrame(() => callback()) runs; update cloneMesh to
check callback and if callback is provided but typeof callback !== 'function'
then log a warning and set callback = null (so only a real function will be
invoked), and apply the same validation to the other occurrence in this file
where requestAnimationFrame(() => callback()) is used (the second callback usage
noted in the review).
In `@api/shapes.js`:
- Around line 704-710: The create3DText branch validates modelId and text but
doesn't guard font before string operations (e.g. font.endsWith(".json")), so
add an early check in the create3DText path: verify font is present and typeof
font === "string" (similar to the text check) and if not, log a warning like
"create3DText: invalid font" and return null; apply the same guard before any
other usage of font (including the later branch that calls
font.endsWith(".json")) so non-string or missing fonts take the graceful-return
path instead of hitting the async loader.
- Around line 611-615: createPlane currently dereferences position without a
safe default, causing createPlane("id") to crash when position is undefined;
update createPlane to normalize position early (before any indexing) by
accepting undefined, array, or Vector3 and converting them to a Vector3 (or
default [0,0,0]) — e.g., add a small normalization step at the top of
createPlane that sets position = normalizePosition(position) or position =
[0,0,0] when falsy, then use the normalized position for indexing; apply the
same normalization pattern to the other similar function referenced (lines
~642-645) so both locations defensively handle undefined, array, and Vector3
inputs.
In `@api/transform.js`:
- Around line 103-111: In positionAt(), ensure x and z are always coerced
through toFinite (use toFinite(x, mesh.position.x) and toFinite(z,
mesh.position.z)) regardless of y === "__ground__level__", and treat y
specially: if y === "__ground__level__" leave the sentinel, otherwise replace y
with a safe numeric fallback via y = toFinite(y, mesh.position.y) (or convert
Number(y) when finite), so no strings/NaN reach the ground raycast or
mesh.position.set; update the code paths around toFinite,
Number.isFinite(Number(y)), and mesh.position.set(...) accordingly.
---
Nitpick comments:
In `@api/shapes.js`:
- Around line 251-261: The parameter destructuring in createBox currently uses
an = {} default which won't guard against callers passing explicit null; change
the signature so you accept an options param and destructure from options ?? {}
(e.g., keep createBox(boxId, options) then const { width = 1, height = 1, depth
= 1, color = "#9932CC", position = new flock.BABYLON.Vector3(0,0,0), alpha = 1,
callback = null } = options ?? {}), and apply the same nullish-coalescing
destructuring fix to the other factory functions with the same pattern shown in
the diff (the later parameter blocks at the lines referenced).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9154fece-0566-4d01-8b3d-af10eebc19dc
📒 Files selected for processing (7)
api/animate.jsapi/control.jsapi/events.jsapi/scene.jsapi/shapes.jsapi/sound.jsapi/transform.js
api/animate.js
Outdated
Comment on lines
+238
to
+241
| duration = Number.isFinite(Number(duration)) && Number(duration) > 0 ? Number(duration) : 1; | ||
| x = Number.isFinite(Number(x)) ? Number(x) : 0; | ||
| y = Number.isFinite(Number(y)) ? Number(y) : 0; | ||
| z = Number.isFinite(Number(z)) ? Number(z) : 0; |
There was a problem hiding this comment.
Don't coerce away __ground__level__.
glideTo(..., { y: "__ground__level__" }) now becomes y = 0 before the sentinel check runs, so valid ground-following calls glide to absolute zero instead of sampled terrain height.
🛠 Suggested fix
- duration = Number.isFinite(Number(duration)) && Number(duration) > 0 ? Number(duration) : 1;
- x = Number.isFinite(Number(x)) ? Number(x) : 0;
- y = Number.isFinite(Number(y)) ? Number(y) : 0;
- z = Number.isFinite(Number(z)) ? Number(z) : 0;
+ duration =
+ Number.isFinite(Number(duration)) && Number(duration) > 0
+ ? Number(duration)
+ : 1;
+ x = Number.isFinite(Number(x)) ? Number(x) : 0;
+ z = Number.isFinite(Number(z)) ? Number(z) : 0;
+ if (y !== "__ground__level__") {
+ y = Number.isFinite(Number(y)) ? Number(y) : 0;
+ }Also applies to: 247-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/animate.js` around lines 238 - 241, The code currently coerces x/y/z to
numbers unconditionally, which destroys the sentinel string "__ground__level__"
used by glideTo; update the coercion so that x, y, and z are left unchanged if
they are the sentinel string (e.g., value === "__ground__level__"), otherwise
coerce to Number and apply the existing numeric fallback (duration keep existing
numeric check). Locate where duration, x, y, z are normalized (variables named
duration, x, y, z inside glideTo/animate) and change those expressions to first
check for the sentinel string, preserving it, then do Number(...)
coercion/fallback; make the same change for the repeated block at lines 247-250.
Comment on lines
+539
to
+547
| cloneMesh({ sourceMeshName, cloneId, callback = null } = {}) { | ||
| if (!sourceMeshName || typeof sourceMeshName !== "string" || sourceMeshName.length > 100) { | ||
| console.warn("cloneMesh: invalid sourceMeshName"); | ||
| return null; | ||
| } | ||
| if (!cloneId || typeof cloneId !== "string" || cloneId.length > 100) { | ||
| console.warn("cloneMesh: invalid cloneId"); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Validate callback alongside the ID fields.
A truthy non-function still reaches requestAnimationFrame(() => callback()) later in this method and throws there. Since this entrypoint is now doing input hardening, it should null out or warn on invalid callbacks as well.
🛠 Suggested fix
cloneMesh({ sourceMeshName, cloneId, callback = null } = {}) {
if (!sourceMeshName || typeof sourceMeshName !== "string" || sourceMeshName.length > 100) {
console.warn("cloneMesh: invalid sourceMeshName");
return null;
}
if (!cloneId || typeof cloneId !== "string" || cloneId.length > 100) {
console.warn("cloneMesh: invalid cloneId");
return null;
}
+ if (callback != null && typeof callback !== "function") {
+ console.warn("cloneMesh: callback must be a function");
+ callback = null;
+ }Also applies to: 595-597
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/scene.js` around lines 539 - 547, cloneMesh currently only validates
sourceMeshName and cloneId but allows a truthy non-function callback that later
causes a runtime error when requestAnimationFrame(() => callback()) runs; update
cloneMesh to check callback and if callback is provided but typeof callback !==
'function' then log a warning and set callback = null (so only a real function
will be invoked), and apply the same validation to the other occurrence in this
file where requestAnimationFrame(() => callback()) is used (the second callback
usage noted in the review).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vite.config.mjs (1)
17-23: UseresolveId+ virtual ID for reliably stubbing missing modules.The current implementation relies on
load(id)which is called only after successful module resolution. If an import fails to resolve,load()is never invoked and the build errors instead of stubbing the missing module. Reliable stubbing requires usingresolveIdto return a virtual module ID (conventionally prefixed with\0) and handling that virtual ID inload().Recommended implementation
const stubMissingBabylonWgslPlugin = { name: "stub-missing-babylon-wgsl", + async resolveId(source, importer) { + if (!/ShadersWGSL|shadersWGSL/.test(source)) return null; + const resolved = await this.resolve(source, importer, { skipSelf: true }); + if (resolved) return null; // keep real module when present + return `\0stub-babylon-wgsl:${source}`; + }, load(id) { - if (id.includes("ShadersWGSL") || id.includes("shadersWGSL")) { + if (id.startsWith("\0stub-babylon-wgsl:")) { return "export default '';"; } + return null; }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite.config.mjs` around lines 17 - 23, The plugin stubMissingBabylonWgslPlugin currently only implements load(id) so it won’t catch imports that fail resolution; change the plugin to implement resolveId(source, importer) and, when source matches "ShadersWGSL" or "shadersWGSL", return a virtual ID (e.g. '\0virtual:ShadersWGSL'); then update load(id) to check for that virtual ID and return "export default '';" so unresolved imports are stubbed reliably. Ensure the plugin name stubMissingBabylonWgslPlugin still remains the same and only the resolveId and load handling are added/adjusted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vite.config.mjs`:
- Around line 12-24: Prettier reported a formatting mismatch in vite.config.mjs;
run your project's Prettier formatter and commit the resulting changes so the
file matches the repo style. Specifically reformat the
stubMissingBabylonWgslPlugin object (its name and load(id) function) to comply
with the project's Prettier rules, ensure consistent quoting/spacing and a final
newline, then add and commit the updated vite.config.mjs.
---
Nitpick comments:
In `@vite.config.mjs`:
- Around line 17-23: The plugin stubMissingBabylonWgslPlugin currently only
implements load(id) so it won’t catch imports that fail resolution; change the
plugin to implement resolveId(source, importer) and, when source matches
"ShadersWGSL" or "shadersWGSL", return a virtual ID (e.g.
'\0virtual:ShadersWGSL'); then update load(id) to check for that virtual ID and
return "export default '';" so unresolved imports are stubbed reliably. Ensure
the plugin name stubMissingBabylonWgslPlugin still remains the same and only the
resolveId and load handling are added/adjusted.
The stub matched all module IDs containing "ShadersWGSL", which incorrectly intercepted greasedLinePluginMaterialShadersWGSL.js — a real file that exports GetCustomCode. Replacing it with an empty stub caused a SyntaxError on the static import, preventing @babylonjs/core from loading and leaving the app stuck on the loading screen. The original ERR_FAILED console messages for missing WGSL shader includes are cosmetic (dynamic imports for WebGPU shaders that fall back silently to WebGL) and are better left alone. https://claude.ai/code/session_01GFLGzae6RMuSSnEo5ax2vH
toDim was clamping any non-positive value to the fallback (1), so create_box with height=0 silently produced a height-1 box. The tallest buildings project relies on boxes starting flat (height 0) and scaling up as data loads. Changing n > 0 to n >= 0 keeps the guard against NaN and negative values while accepting zero as a valid dimension. https://claude.ai/code/session_01GFLGzae6RMuSSnEo5ax2vH
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/shapes.js (1)
1-1:⚠️ Potential issue | 🟡 MinorPipeline: Prettier formatting check failed.
The CI reports a formatting issue. Run
prettier --write api/shapes.jsto fix before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/shapes.js` at line 1, Run Prettier to fix formatting in api/shapes.js (the file containing the import earcut statement); execute prettier --write api/shapes.js (or run your project's formatting script) and commit the resulting changes so the import line and the rest of the file conform to the project's Prettier rules.
♻️ Duplicate comments (2)
api/shapes.js (2)
703-710:⚠️ Potential issue | 🟠 Major
fontstill not validated before string operations.The past review comment about this remains unaddressed. If
fontis omitted or non-string, line 723 (font.endsWith(".json")) throws aTypeError. While caught by the try-catch, it produces a confusing error message and incorrect fallback behavior.🛡️ Proposed fix: validate font early
if (!validateShapeId(modelId, "create3DText")) return null; if (!text || typeof text !== "string") { console.warn("create3DText: invalid text"); return null; } + if (!font || typeof font !== "string") { + console.warn("create3DText: invalid font"); + return null; + } size = toDim(size, 50); depth = toDim(depth, 1);Also applies to: 722-723
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/shapes.js` around lines 703 - 710, In create3DText, validate the font variable before any string operations: ensure font is present and typeof font === "string" (or coerce to a safe default) before calling font.endsWith(".json") so the function doesn't throw a TypeError; update the logic around the font variable (the block using font.endsWith(".json") and any fallbacks) to first check the string type and then proceed to JSON-checking or defaulting, and keep validateShapeId and existing size/depth handling unchanged.
611-614:⚠️ Potential issue | 🔴 Critical
positionstill lacks a default, causing crashes when omitted.The past review comment about this issue remains unaddressed. Calling
createPlane("id")orcreatePlane("id", { color: "red" })will throw aTypeErrorat line 643 when accessingposition[0]onundefined.🐛 Proposed fix: add default for position
- createPlane(planeId, { color, width, height, position, callback = null } = {}) { + createPlane(planeId, { color, width, height, position = [0, 0, 0], callback = null } = {}) {Also applies to: 642-646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/shapes.js` around lines 611 - 614, The createPlane function's destructured parameters lack a default for position causing a TypeError when callers omit it; update the parameter destructuring for createPlane(planeId, { color, width, height, position = [0,0,0], callback = null } = {}) or, at the top of the function (after validateShapeId and toDim calls), ensure position is set to a safe default like [0,0,0] before any access to position[0]; reference createPlane, validateShapeId, and toDim to locate the code and make the change so position is never undefined when used.
🧹 Nitpick comments (1)
api/shapes.js (1)
267-267: Consider extracting alpha clamping to a helper for DRY.The same alpha clamping logic is repeated in
createBox,createSphere,createCylinder, andcreateCapsule. A small helper would reduce duplication and risk of inconsistency.♻️ Optional helper extraction
+// Coerce alpha to a finite number in [0, 1], defaulting to 1. +function toAlpha(v) { + const n = Number(v); + return Number.isFinite(n) ? Math.max(0, Math.min(1, n)) : 1; +}Then replace each occurrence:
-alpha = Number.isFinite(Number(alpha)) ? Math.max(0, Math.min(1, Number(alpha))) : 1; +alpha = toAlpha(alpha);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/shapes.js` at line 267, Extract the repeated alpha normalization/clamping logic into a single helper (e.g., normalizeAlpha or clampAlpha) and replace the inlined line in createBox, createSphere, createCylinder, and createCapsule with a call to that helper; the helper should accept the raw alpha value, coerce to Number, return 1 for non-finite inputs, and otherwise return Math.max(0, Math.min(1, Number(alpha))). Ensure all four functions import/require or reference the new helper to remove duplication and keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/shapes.js`:
- Line 1: Run Prettier to fix formatting in api/shapes.js (the file containing
the import earcut statement); execute prettier --write api/shapes.js (or run
your project's formatting script) and commit the resulting changes so the import
line and the rest of the file conform to the project's Prettier rules.
---
Duplicate comments:
In `@api/shapes.js`:
- Around line 703-710: In create3DText, validate the font variable before any
string operations: ensure font is present and typeof font === "string" (or
coerce to a safe default) before calling font.endsWith(".json") so the function
doesn't throw a TypeError; update the logic around the font variable (the block
using font.endsWith(".json") and any fallbacks) to first check the string type
and then proceed to JSON-checking or defaulting, and keep validateShapeId and
existing size/depth handling unchanged.
- Around line 611-614: The createPlane function's destructured parameters lack a
default for position causing a TypeError when callers omit it; update the
parameter destructuring for createPlane(planeId, { color, width, height,
position = [0,0,0], callback = null } = {}) or, at the top of the function
(after validateShapeId and toDim calls), ensure position is set to a safe
default like [0,0,0] before any access to position[0]; reference createPlane,
validateShapeId, and toDim to locate the code and make the change so position is
never undefined when used.
---
Nitpick comments:
In `@api/shapes.js`:
- Line 267: Extract the repeated alpha normalization/clamping logic into a
single helper (e.g., normalizeAlpha or clampAlpha) and replace the inlined line
in createBox, createSphere, createCylinder, and createCapsule with a call to
that helper; the helper should accept the raw alpha value, coerce to Number,
return 1 for non-finite inputs, and otherwise return Math.max(0, Math.min(1,
Number(alpha))). Ensure all four functions import/require or reference the new
helper to remove duplication and keep behavior identical.
Same n > 0 guard as toDim was silently converting scale Y=0 to 1, preventing the tallest buildings project from starting boxes flat. https://claude.ai/code/session_01GFLGzae6RMuSSnEo5ax2vH
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/transform.js (1)
103-111:⚠️ Potential issue | 🟠 Major
positionAt()still leaks unsanitized coordinates.Line 107 still gates
x/zcoercion on the non-sentinel path, and Line 110 leaves an invalid non-sentinelyuntouched.setBlockPositionOnMesh()can still receive strings/NaNfor the ground lookup ormesh.position.set(...).🛠️ Suggested fix
- x = x ?? mesh.position.x; - y = y ?? mesh.position.y; - z = z ?? mesh.position.z; - // Allow the ground-level sentinel string through; coerce everything else. - if (y !== "__ground__level__") { - x = toFinite(x, mesh.position.x); - z = toFinite(z, mesh.position.z); - if (Number.isFinite(Number(y))) y = Number(y); - } + x = toFinite(x ?? mesh.position.x, mesh.position.x); + z = toFinite(z ?? mesh.position.z, mesh.position.z); + y = y ?? mesh.position.y; + if (y !== "__ground__level__") { + y = toFinite(y, mesh.position.y); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/transform.js` around lines 103 - 111, The positionAt() logic still lets non-sentinel strings/NaN through to setBlockPositionOnMesh() and mesh.position.set(...); always coerce x,z (and y when not the "__ground__level__" sentinel) using toFinite with mesh.position.x/y/z defaults before any sentinel check, and ensure y is converted to a Number and then toFinite (so non-numeric strings become the mesh.position.y default rather than remaining as strings/NaN). Update the block around x = x ?? mesh.position.x; y = y ?? mesh.position.y; z = z ?? mesh.position.z; to call toFinite(x, mesh.position.x) and toFinite(z, mesh.position.z) unconditionally, and for y: if y === "__ground__level__" keep the sentinel, else set y = toFinite(Number(y), mesh.position.y); this prevents setBlockPositionOnMesh() and mesh.position.set from receiving invalid types.
🧹 Nitpick comments (1)
api/transform.js (1)
625-627: Guard against explicitly null or empty-string scale values before coercion.Lines 625–627 coerce values to numbers without checking for null or blank strings, so explicitly passing
scale(meshName, {x: null})orscale(meshName, {x: ""})would flatten the axis instead of preserving the default 1. While the current destructuring defaults handle omitted parameters correctly, defensive checks for explicit null and empty strings would prevent accidental misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/transform.js` around lines 625 - 627, The numeric coercion for x, y, z in transform.js currently converts null or empty-string to 0 (flattening the axis); update the guards in the scale logic (the x, y, z variables used by the scale function) to treat explicit null and "" as “use default 1” before coercion—i.e. check x === null || x === "" (and same for y and z) and fallback to 1, otherwise run the existing Number.isFinite(Number(...)) && Number(...) >= 0 check to coerce/validate the provided value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api/transform.js`:
- Around line 103-111: The positionAt() logic still lets non-sentinel
strings/NaN through to setBlockPositionOnMesh() and mesh.position.set(...);
always coerce x,z (and y when not the "__ground__level__" sentinel) using
toFinite with mesh.position.x/y/z defaults before any sentinel check, and ensure
y is converted to a Number and then toFinite (so non-numeric strings become the
mesh.position.y default rather than remaining as strings/NaN). Update the block
around x = x ?? mesh.position.x; y = y ?? mesh.position.y; z = z ??
mesh.position.z; to call toFinite(x, mesh.position.x) and toFinite(z,
mesh.position.z) unconditionally, and for y: if y === "__ground__level__" keep
the sentinel, else set y = toFinite(Number(y), mesh.position.y); this prevents
setBlockPositionOnMesh() and mesh.position.set from receiving invalid types.
---
Nitpick comments:
In `@api/transform.js`:
- Around line 625-627: The numeric coercion for x, y, z in transform.js
currently converts null or empty-string to 0 (flattening the axis); update the
guards in the scale logic (the x, y, z variables used by the scale function) to
treat explicit null and "" as “use default 1” before coercion—i.e. check x ===
null || x === "" (and same for y and z) and fallback to 1, otherwise run the
existing Number.isFinite(Number(...)) && Number(...) >= 0 check to
coerce/validate the provided value.
- createPlane: default position to [0,0,0] to prevent crash when omitted - glideTo: preserve __ground__level__ sentinel before numeric coercion - positionAt: always coerce x/z; only coerce y when not the sentinel - create3DText: validate font before calling .endsWith() - easing check: guard with instanceof EasingFunction to reject non-easing constructors - wait(): clamp ms to 2147483647 to avoid silent setTimeout overflow - cloneMesh: null out non-function callbacks with a warning - shapes.js: extract toAlpha() helper, replace four inline copies https://claude.ai/code/session_01GFLGzae6RMuSSnEo5ax2vH
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/transform.js (1)
621-623: Consider extracting the coerced value to avoid redundantNumber()calls.The validation logic is correct (non-negative, finite, default to 1), but
Number(x)is evaluated up to 4 times per variable. This is a minor inefficiency.♻️ Suggested refactor
- x = Number.isFinite(Number(x)) && Number(x) >= 0 ? Number(x) : 1; - y = Number.isFinite(Number(y)) && Number(y) >= 0 ? Number(y) : 1; - z = Number.isFinite(Number(z)) && Number(z) >= 0 ? Number(z) : 1; + { + const nx = Number(x), ny = Number(y), nz = Number(z); + x = Number.isFinite(nx) && nx >= 0 ? nx : 1; + y = Number.isFinite(ny) && ny >= 0 ? ny : 1; + z = Number.isFinite(nz) && nz >= 0 ? nz : 1; + }Alternatively, reuse the
toDimhelper pattern with a custom fallback of1:function toNonNegative(v, fallback) { const n = Number(v); return Number.isFinite(n) && n >= 0 ? n : fallback; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/transform.js` around lines 621 - 623, Extract the repeated Number() coercion by computing a single numeric value per variable and reuse it (or add a small helper like toNonNegative(v, fallback)) and then assign x, y, z using that single numeric check; e.g., for each of x, y, z compute const nx = Number(x) (or call toNonNegative(x, 1)), test Number.isFinite(nx) && nx >= 0, and set x = nx or fallback 1, and do the same for y and z to remove the redundant Number() calls in the assignments shown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/transform.js`:
- Around line 621-623: Extract the repeated Number() coercion by computing a
single numeric value per variable and reuse it (or add a small helper like
toNonNegative(v, fallback)) and then assign x, y, z using that single numeric
check; e.g., for each of x, y, z compute const nx = Number(x) (or call
toNonNegative(x, 1)), test Number.isFinite(nx) && nx >= 0, and set x = nx or
fallback 1, and do the same for y and z to remove the redundant Number() calls
in the assignments shown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36467c32-4025-40ef-afcc-7b35f72984ab
📒 Files selected for processing (5)
api/animate.jsapi/control.jsapi/scene.jsapi/shapes.jsapi/transform.js
🚧 Files skipped from review as they are similar to previous changes (2)
- api/scene.js
- api/control.js
This was referenced Mar 26, 2026
Merged
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
2 participants
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.
Summary
This PR adds comprehensive input validation and type coercion across multiple API modules to improve robustness and prevent runtime errors from invalid or unexpected parameter values.
Key Changes
api/shapes.js
validateShapeId()helper to validate shape/mesh IDs (non-empty string, max 100 chars)toDim()helper to coerce dimension values to positive finite numbers with fallbackcreateBox,createSphere,createCylinder,createCapsule,createPlane, andcreate3DText= {}for optional parameterscreate3DTextapi/transform.js
toFinite()helper to coerce values to finite numbers with fallbacksetBlockPositionOnMesh()to validate and coerce x, y, z coordinates while preserving the"__ground__level__"sentinel valuemoveByVector(),rotate(),rotateTo(), andscale()methodsapi/animate.js
animateRotation()andanimateGlide()api/sound.js
playSound()setBPM()(defaults to 60)setBPM()from throwing to warning and graceful returnapi/events.js
onEvent(),whenActionEvent(), andwhenKeyEvent()api/scene.js
sourceMeshNameandcloneIdparameters incloneMesh()api/control.js
wait()method (non-negative finite number)waitUntil()Implementation Details
Number.isFinite()for robust numeric checksconsole.warn()for debuggingNumber()constructor followed by range/type validationhttps://claude.ai/code/session_01GFLGzae6RMuSSnEo5ax2vH
Summary by CodeRabbit
Bug Fixes
Refactor