Conversation
📝 WalkthroughWalkthroughThis PR introduces 15 new generator modules that enable Blockly-to-JavaScript code generation for various block categories including animations, scene management, control flow, data structures, events, materials, sounds, text/UI, transforms, and utilities. It establishes a comprehensive compilation infrastructure with shared mesh state tracking and utility functions for code safety and variable resolution. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (11)
generators/mesh-state.js (1)
37-48: Consider resettinguniqueIdCounterinclearMeshMaps.
clearMeshMaps()clears the mesh maps but doesn't resetuniqueIdCounter. If a fresh code generation session is intended after clearing, IDs will continue from where they left off (e.g.,_47,_48...). This may be intentional for uniqueness across sessions, but if each session should start fresh, the counter should reset.Optional: Reset counter with maps
export function clearMeshMaps() { for (const key of Object.keys(_meshMap)) delete meshMap[key]; for (const key of Object.keys(_meshBlockIdMap)) delete meshBlockIdMap[key]; + uniqueIdCounter = 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generators/mesh-state.js` around lines 37 - 48, clearMeshMaps currently clears _meshMap and _meshBlockIdMap but does not reset uniqueIdCounter, so subsequent generateUniqueId calls continue from the old counter; update clearMeshMaps to also reset uniqueIdCounter to 0 (or desired base) so generateUniqueId(prefix) starts fresh after clearing. Modify the clearMeshMaps function to set uniqueIdCounter = 0 alongside the existing map deletions; keep generateUniqueId and the counter variable names unchanged.generators/generators-control.js (1)
158-158: Preferletovervarfor consistency.Line 158 uses
varforlistVar, while the rest of the codebase usesletfor loop variables and declarations. Usingletprovides block scoping and aligns with the modern JS patterns used elsewhere in these generators.Proposed fix
- code += "var " + listVar + " = " + argument0 + ";\n"; + code += "let " + listVar + " = " + argument0 + ";\n";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generators/generators-control.js` at line 158, Replace the use of `var` with `let` when declaring the generated list variable: locate the string assembly where `code += "var " + listVar + " = " + argument0 + ";\n";` is built in generators-control.js and change it to use `let` for `listVar` so the generated code uses block-scoped declarations consistent with the rest of the codebase.generators/generators-text.js (1)
382-385: Redundant font assignment.The conditional on line 383-384 sets
fontto the same value it was already assigned on line 382. This appears to be placeholder code for future font options but currently does nothing.Suggested simplification (or add TODO)
- let font = "./fonts/FreeSans_Bold.json"; - if (fontKey === "__fonts_FreeSans_Bold_json") - font = "./fonts/FreeSans_Bold.json"; + // TODO: Add support for additional fonts based on fontKey + const font = "./fonts/FreeSans_Bold.json";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generators/generators-text.js` around lines 382 - 385, The code sets let font = "./fonts/FreeSans_Bold.json" then immediately reassigns the same value inside an if (fontKey === "__fonts_FreeSans_Bold_json") block; remove the redundant conditional assignment or replace it with a meaningful branch/TODO. Specifically, in the generator where the variable font and the condition on fontKey are used, delete the duplicate assignment inside the if or convert the if to handle additional fontKey values (or add a clear TODO comment) so the conditional no longer redundantly reassigns the same "./fonts/FreeSans_Bold.json" value.generators/generators-math.js (1)
51-55: Consider specifying radix forparseInt.
parseIntwithout a radix can produce unexpected results for strings with leading zeros (e.g.,parseInt("08")historically returned0in older engines). Specifying radix 10 makes the intent explicit and avoids edge-case surprises.Proposed fix
if (conversionType === "INT") { - code = `parseInt(${string})`; + code = `parseInt(${string}, 10)`; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generators/generators-math.js` around lines 51 - 55, The parseInt call in the conversion branch for conversionType === "INT" should explicitly pass a radix to avoid legacy/locale parsing issues; update the code generation in generators/generators-math.js so the INT branch produces parseInt(${string}, 10) instead of parseInt(${string}), leaving the FLOAT branch as parseFloat(${string}); ensure you modify the template where code is assigned for the INT case (the variable named code in that conditional).generators/generators-sensing.js (1)
98-101: Use strict equality for string comparison.Using
===instead of==for the string comparison is more explicit and avoids potential type coercion issues.Proposed fix
javascriptGenerator.forBlock["canvas_controls"] = function (block) { - const controls = block.getFieldValue("CONTROLS") == "TRUE"; + const controls = block.getFieldValue("CONTROLS") === "TRUE"; return `canvasControls(${controls});\n`; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generators/generators-sensing.js` around lines 98 - 101, In javascriptGenerator.forBlock["canvas_controls"] change the string comparison to use strict equality: when assigning controls use block.getFieldValue("CONTROLS") === "TRUE" (inside the canvas_controls generator function and the controls variable) so it avoids type coercion and is explicit; update the controls assignment accordingly and keep the returned template string unchanged.generators/generators-data.js (2)
172-184: Closure mutation pattern incacheList()is intentional but worth documenting.The
cacheList()inner function mutates the outerlistvariable via closure side effect. This is a valid caching pattern (similar to Blockly's own generators), but a brief inline comment would improve maintainability for future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generators/generators-data.js` around lines 172 - 184, The inner function cacheList mutates the outer variable list via closure for caching; add a brief inline comment above or inside cacheList explaining this intentional closure mutation pattern (e.g., "Intentionally mutates outer `list` variable to cache a temporary variable name for generated code") and reference that it returns the listAssignment while updating list; keep the comment near the cacheList definition and mention the relation to javascriptGenerator.nameDB_.getDistinctName and the returned listAssignment so future readers understand the side-effect.
54-57: Minor: Inconsistent indentation in generated code.The return statement has misaligned closing brace and extra trailing whitespace which produces inconsistent indentation in the generated JavaScript.
🔧 Suggested fix
- return `if (Array.isArray(${listName})) { - ${listName}.splice(${index}, 1); - } - `; + return `if (Array.isArray(${listName})) {\n ${listName}.splice(${index}, 1);\n}\n`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generators/generators-data.js` around lines 54 - 57, The generated code string for the array removal block returns misaligned indentation and an extra trailing space; update the return template that produces the string containing "if (Array.isArray(${listName})) { ... }" so the closing brace lines up with the opening line and remove the trailing whitespace/newline at the end — i.e., produce exactly: if (Array.isArray(${listName})) {\n ${listName}.splice(${index}, 1);\n}\n (adjust the template in the function that constructs this return using the listName and index variables).generators/generators-sound.js (1)
245-255: Fragile field navigation pattern for mesh input.Navigating through
meshInput.fieldRow.find()is brittle and could break if the block structure changes. Consider usingblock.getFieldValue("MESH_NAME")directly if the field is defined on the block, or document why this indirect approach is necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generators/generators-sound.js` around lines 245 - 255, The current code uses a brittle traversal (meshInput.fieldRow.find(...) via meshInput and meshDropdownField) to read the mesh name; replace this with block.getFieldValue("MESH_NAME") to reliably obtain the field value (falling back to "__everywhere__" when null/empty) and then build meshVariable from that value (preserving the existing behavior of wrapping the value in quotes). Update references around meshInput, meshDropdownField, meshValue, and meshVariable so the field lookup uses block.getFieldValue("MESH_NAME") and remove the fragile fieldRow navigation.generators/generators-scene.js (1)
422-454: Inconsistent use ofparseFloatwrapping.
emitRate,startAlpha, andendAlphaare wrapped withparseFloat()at code-generation time, whileminSize,maxSize,minLifetime,maxLifetime, etc. are not. SincevalueToCodereturns a string representation of the code (not the actual value), callingparseFloat()here converts the string"10"to the number10, which then gets embedded directly in the output. This works but is inconsistent with other parameters. Consider either removing theparseFloat()calls (letting the runtime handle numeric conversion) or applying it uniformly.🔧 Suggested fix - remove parseFloat for consistency
- const emitRate = parseFloat( - javascriptGenerator.valueToCode( + const emitRate = + javascriptGenerator.valueToCode( block, "RATE", javascriptGenerator.ORDER_ATOMIC, - ) || "10", - ); + ) || "10";Apply similar changes to
startAlphaandendAlpha.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generators/generators-scene.js` around lines 422 - 454, The generator inconsistently wraps numeric fields at generation time (emitRate, startAlpha, endAlpha) with parseFloat, embedding literal numbers instead of leaving expressions returned by javascriptGenerator.valueToCode; remove the parseFloat(...) calls around javascriptGenerator.valueToCode(...) for emitRate, startAlpha, and endAlpha so they behave like minSize/maxSize/minLifetime/maxLifetime and let the runtime handle numeric conversion, keeping the valueToCode output as the generated code string.generators/generators-animate.js (2)
199-218: Default values for vector coordinates use numeric0instead of string"0".When
valueToCodereturns empty, the fallback is numeric0instead of string"0". While this works due to JavaScript's type coercion in template literals, it's inconsistent with the pattern used elsewhere (e.g.,generators-transform.jsuses"0"). Consider using string"0"for consistency.🔧 Suggested fix
const x = javascriptGenerator.valueToCode( valueBlock, "X", javascriptGenerator.ORDER_ATOMIC, - ) || 0; + ) || "0";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generators/generators-animate.js` around lines 199 - 218, The vector coordinate fallbacks in generators-animate.js use numeric 0 which is inconsistent with other generators; update the three fallbacks for x, y, and z (the variables assigned from javascriptGenerator.valueToCode calls) to use the string "0" instead of the numeric 0 so they match the pattern used elsewhere (keep the same javascriptGenerator.valueToCode(...) calls and ORDER_ATOMIC arguments and only change the fallback values).
162-280: Significant code duplication betweenanimationandanimate_keyframesgenerators.Both generators share nearly identical keyframe extraction logic (iterating through
inputList, handling color/vector/scalar properties, building keyframes array). Consider extracting the common keyframe-parsing logic into a shared helper function ingenerators-utilities.jsto reduce duplication and maintenance burden.Also applies to: 283-381
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generators/generators-animate.js` around lines 162 - 280, The animation generator (javascriptGenerator.forBlock["animation"]) duplicates the keyframe-parsing logic also used by the animate_keyframes generator; extract that shared logic into a helper in generators-utilities.js (e.g., parseKeyframesFromBlock(block, inputName, property)) that iterates the inputList, resolves value and duration blocks, handles colour/color, position/rotation/scaling vectors (returning createVector3(...) when needed), and returns a normalized keyframes array (or the code-ready keyframes string); then replace the inlined loop in both javascriptGenerator.forBlock["animation"] and the animate_keyframes generator to call parseKeyframesFromBlock and use its result to build keyframesCode, keeping existing field names like PROPERTY, KEYFRAMES, X/Y/Z, VALUE, DURATION, and preserving defaults ("#ffffff", createVector3(0,0,0), duration "1").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@generators/generators-control.js`:
- Around line 177-185: The generated loop uses "for (... in ...)" which can
iterate non-numeric keys; change the string construction in
generators-control.js that builds the loop (the block using variables code,
indexVar, listVar and branch) to emit a traditional indexed loop or a for...of
pattern instead—e.g., produce "for (var {indexVar} = 0; {indexVar} <
{listVar}.length; {indexVar}++) { ... await wait(0); }" or emit "for (const
{indexVar} of {listVar}) { ... await wait(0); }" so iteration is over actual
array elements/indices and not enumerable property keys; keep branch and the
await wait(0) inside the loop.
In `@generators/generators-deprecated.js`:
- Around line 459-463: The get_lexical_variable generator is returning the raw
field value from block.getFieldValue("VAR") instead of resolving/sanitizing it
via the name database; update
javascriptGenerator.forBlock["get_lexical_variable"] to call the name resolution
helper (nameDB_.getName with the appropriate variable name type/constant) on the
retrieved variableName and return that resolved identifier with
javascriptGenerator.ORDER_ATOMIC so reserved words/special chars are handled
correctly.
- Around line 158-170: The generator
javascriptGenerator.forBlock["create_custom_map"] is using valueToCode(...) ||
"#808080" which produces unquoted token output; change the fallback to a quoted
JS string (e.g. use a fallback like "\"#808080\"" so the generated array
elements are valid string literals), update the code path where COLOR_i is
computed (the valueToCode calls) so defaults include surrounding quotes before
joining and passing into createCustomMap.
In `@generators/generators-events.js`:
- Around line 112-113: Call to emitSafeIdentifierLiteral in the broadcastEvent
generation includes an extraneous second parameter; change the call in
generators-events.js from emitSafeIdentifierLiteral(raw, undefined) to
emitSafeIdentifierLiteral(raw) so it matches the function signature in
generators-utilities.js (keep the surrounding const safe = ... and return
`broadcastEvent(${safe});\n` intact).
- Around line 63-68: Remove the unsupported third boolean argument passed to
nameDB_.getName() in the collision block generator: in
javascriptGenerator.forBlock["on_collision"] (and the similar calls creating
otherModelName) call javascriptGenerator.nameDB_.getName with only two arguments
(the result of block.getFieldValue("MODEL_VAR") or the other field and
Blockly.Names.NameType.VARIABLE) — do the same cleanup for equivalent getName
calls in generators-deprecated.js so no getName invocation passes a third
parameter.
- Around line 34-41: The generator loop uses block.thenCount_ which is never
initialized so the for-loop never runs; either initialize this property in the
block definition (e.g., set this.thenCount_ = 0 in the block's init() and update
it whenever THEN inputs are added/removed) or stop using it and instead iterate
over the actual THEN inputs dynamically (e.g., detect "THEN0", "THEN1", ...
until statementToCode returns empty). Update the block definition/init() to
define thenCount_ or replace the for (let i = 0; i < block.thenCount_; i++) loop
in generators/generators-events.js to iterate based on existing inputs via
javascriptGenerator.statementToCode(block, "THEN"+i) until no code is returned.
In `@generators/generators-functions.js`:
- Around line 90-92: The loop accesses block.arguments_ directly which can be
undefined; change the variables initialization to be null-safe (e.g., set
variables = block.arguments_ || [] or guard before the loop) so the for loop
over variables (used to build args) never throws when block.arguments_ is
missing; update the code paths that reference variables/args accordingly (look
for variables and args in this block to apply the same null-safety).
- Around line 22-27: The statementToCode call in generators-functions.js is
being invoked with an extra third argument (javascriptGenerator.ORDER_NONE)
which the API does not accept; remove the third parameter from the call to
javascriptGenerator.statementToCode(block, "STACK") and do the same for the
other occurrences in this file (e.g., the similar call around the branch/STACK
logic and the one near lines 50-51) so each invocation uses only (block, name).
In `@generators/generators-text.js`:
- Around line 67-76: The generated say(...) call is inserting raw color strings
unquoted (textColor: `#000000`), causing invalid JS; update the code that builds
the return string in generators-text.js to ensure color values are properly
quoted—e.g., replace ${textColor} and ${backgroundColor} with
${JSON.stringify(textColor)} (or otherwise wrap them in quotes) when
constructing the say(...) argument so textColor and backgroundColor become valid
string literals; locate the block where textColor/backgroundColor are read from
getFieldValue and adjust the interpolation in the return string for say(...)
accordingly.
In `@generators/generators-transform.js`:
- Around line 176-180: The defaults for xOrigin/yOrigin/zOrigin are currently
set to "'CENTRE'", causing double quotes when interpolated; change the defaults
to unquoted strings (e.g. "CENTRE") by using xOrigin =
block.getFieldValue("X_ORIGIN") || "CENTRE" (same for yOrigin and zOrigin) and
ensure the template interpolation in the return (the scale call using modelName
and xOrigin/yOrigin/zOrigin) does not add extra quotes around those variables so
the generated code becomes xOrigin: 'CENTRE' only once; update references in
this file for xOrigin, yOrigin, zOrigin and the return that builds the
scale(...) call accordingly.
- Around line 25-42: The switch in
javascriptGenerator.forBlock["move_by_xyz_single"] can return undefined for
unexpected coordinate values; add a default case to the switch on coordinate
(which uses getFieldValue and modelName) that provides a sensible fallback or
explicit error handling—for example, fall back to x (call moveByVector with x:
value, y:0, z:0) or throw/log an error via the same code path—so the function
always returns a valid string instead of undefined.
In `@generators/generators-utilities.js`:
- Around line 91-109: The createMesh function currently declares (block,
meshType, params) but callers pass a fourth argument (a mesh type prefix); add a
fourth parameter (e.g., meshPrefix) to the createMesh signature and use it where
the mesh identity is built (for example incorporate meshPrefix into meshId or
into the create${meshType} call) so the passed prefix is not silently ignored;
alternatively, if the prefix is truly unnecessary, remove the extra argument
from all call sites in generators/generators-scene.js. Ensure the change
references the createMesh function and the meshId construction (variableName,
userVariableName, block.id) so callers and behavior remain consistent.
---
Nitpick comments:
In `@generators/generators-animate.js`:
- Around line 199-218: The vector coordinate fallbacks in generators-animate.js
use numeric 0 which is inconsistent with other generators; update the three
fallbacks for x, y, and z (the variables assigned from
javascriptGenerator.valueToCode calls) to use the string "0" instead of the
numeric 0 so they match the pattern used elsewhere (keep the same
javascriptGenerator.valueToCode(...) calls and ORDER_ATOMIC arguments and only
change the fallback values).
- Around line 162-280: The animation generator
(javascriptGenerator.forBlock["animation"]) duplicates the keyframe-parsing
logic also used by the animate_keyframes generator; extract that shared logic
into a helper in generators-utilities.js (e.g., parseKeyframesFromBlock(block,
inputName, property)) that iterates the inputList, resolves value and duration
blocks, handles colour/color, position/rotation/scaling vectors (returning
createVector3(...) when needed), and returns a normalized keyframes array (or
the code-ready keyframes string); then replace the inlined loop in both
javascriptGenerator.forBlock["animation"] and the animate_keyframes generator to
call parseKeyframesFromBlock and use its result to build keyframesCode, keeping
existing field names like PROPERTY, KEYFRAMES, X/Y/Z, VALUE, DURATION, and
preserving defaults ("#ffffff", createVector3(0,0,0), duration "1").
In `@generators/generators-control.js`:
- Line 158: Replace the use of `var` with `let` when declaring the generated
list variable: locate the string assembly where `code += "var " + listVar + " =
" + argument0 + ";\n";` is built in generators-control.js and change it to use
`let` for `listVar` so the generated code uses block-scoped declarations
consistent with the rest of the codebase.
In `@generators/generators-data.js`:
- Around line 172-184: The inner function cacheList mutates the outer variable
list via closure for caching; add a brief inline comment above or inside
cacheList explaining this intentional closure mutation pattern (e.g.,
"Intentionally mutates outer `list` variable to cache a temporary variable name
for generated code") and reference that it returns the listAssignment while
updating list; keep the comment near the cacheList definition and mention the
relation to javascriptGenerator.nameDB_.getDistinctName and the returned
listAssignment so future readers understand the side-effect.
- Around line 54-57: The generated code string for the array removal block
returns misaligned indentation and an extra trailing space; update the return
template that produces the string containing "if (Array.isArray(${listName})) {
... }" so the closing brace lines up with the opening line and remove the
trailing whitespace/newline at the end — i.e., produce exactly: if
(Array.isArray(${listName})) {\n ${listName}.splice(${index}, 1);\n}\n (adjust
the template in the function that constructs this return using the listName and
index variables).
In `@generators/generators-math.js`:
- Around line 51-55: The parseInt call in the conversion branch for
conversionType === "INT" should explicitly pass a radix to avoid legacy/locale
parsing issues; update the code generation in generators/generators-math.js so
the INT branch produces parseInt(${string}, 10) instead of parseInt(${string}),
leaving the FLOAT branch as parseFloat(${string}); ensure you modify the
template where code is assigned for the INT case (the variable named code in
that conditional).
In `@generators/generators-scene.js`:
- Around line 422-454: The generator inconsistently wraps numeric fields at
generation time (emitRate, startAlpha, endAlpha) with parseFloat, embedding
literal numbers instead of leaving expressions returned by
javascriptGenerator.valueToCode; remove the parseFloat(...) calls around
javascriptGenerator.valueToCode(...) for emitRate, startAlpha, and endAlpha so
they behave like minSize/maxSize/minLifetime/maxLifetime and let the runtime
handle numeric conversion, keeping the valueToCode output as the generated code
string.
In `@generators/generators-sensing.js`:
- Around line 98-101: In javascriptGenerator.forBlock["canvas_controls"] change
the string comparison to use strict equality: when assigning controls use
block.getFieldValue("CONTROLS") === "TRUE" (inside the canvas_controls generator
function and the controls variable) so it avoids type coercion and is explicit;
update the controls assignment accordingly and keep the returned template string
unchanged.
In `@generators/generators-sound.js`:
- Around line 245-255: The current code uses a brittle traversal
(meshInput.fieldRow.find(...) via meshInput and meshDropdownField) to read the
mesh name; replace this with block.getFieldValue("MESH_NAME") to reliably obtain
the field value (falling back to "__everywhere__" when null/empty) and then
build meshVariable from that value (preserving the existing behavior of wrapping
the value in quotes). Update references around meshInput, meshDropdownField,
meshValue, and meshVariable so the field lookup uses
block.getFieldValue("MESH_NAME") and remove the fragile fieldRow navigation.
In `@generators/generators-text.js`:
- Around line 382-385: The code sets let font = "./fonts/FreeSans_Bold.json"
then immediately reassigns the same value inside an if (fontKey ===
"__fonts_FreeSans_Bold_json") block; remove the redundant conditional assignment
or replace it with a meaningful branch/TODO. Specifically, in the generator
where the variable font and the condition on fontKey are used, delete the
duplicate assignment inside the if or convert the if to handle additional
fontKey values (or add a clear TODO comment) so the conditional no longer
redundantly reassigns the same "./fonts/FreeSans_Bold.json" value.
In `@generators/mesh-state.js`:
- Around line 37-48: clearMeshMaps currently clears _meshMap and _meshBlockIdMap
but does not reset uniqueIdCounter, so subsequent generateUniqueId calls
continue from the old counter; update clearMeshMaps to also reset
uniqueIdCounter to 0 (or desired base) so generateUniqueId(prefix) starts fresh
after clearing. Modify the clearMeshMaps function to set uniqueIdCounter = 0
alongside the existing map deletions; keep generateUniqueId and the counter
variable names unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 757c8c27-a03a-4580-a000-b059e2ccd52c
📒 Files selected for processing (17)
generators/generators-animate.jsgenerators/generators-condition.jsgenerators/generators-control.jsgenerators/generators-data.jsgenerators/generators-deprecated.jsgenerators/generators-events.jsgenerators/generators-functions.jsgenerators/generators-material.jsgenerators/generators-math.jsgenerators/generators-scene.jsgenerators/generators-sensing.jsgenerators/generators-sound.jsgenerators/generators-text.jsgenerators/generators-transform.jsgenerators/generators-utilities.jsgenerators/generators.jsgenerators/mesh-state.js
| code += | ||
| "for (var " + | ||
| indexVar + | ||
| " in " + | ||
| listVar + | ||
| ") {\n" + | ||
| branch + | ||
| "\n await wait(0);\n" + | ||
| "}\n"; |
There was a problem hiding this comment.
for...in is problematic for array iteration.
for...in iterates over enumerable property keys (including inherited ones) as strings, not array indices. For arrays, this can produce unexpected results if the array has additional properties or if the prototype is modified. Consider using a traditional indexed for loop or for...of.
Proposed fix using indexed for loop
- code +=
- "for (var " +
- indexVar +
- " in " +
- listVar +
- ") {\n" +
- branch +
- "\n await wait(0);\n" +
- "}\n";
+ code +=
+ "for (let " +
+ indexVar +
+ " = 0; " +
+ indexVar +
+ " < " +
+ listVar +
+ ".length; " +
+ indexVar +
+ "++) {\n" +
+ branch +
+ "\n await wait(0);\n" +
+ "}\n";📝 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.
| code += | |
| "for (var " + | |
| indexVar + | |
| " in " + | |
| listVar + | |
| ") {\n" + | |
| branch + | |
| "\n await wait(0);\n" + | |
| "}\n"; | |
| code += | |
| "for (let " + | |
| indexVar + | |
| " = 0; " + | |
| indexVar + | |
| " < " + | |
| listVar + | |
| ".length; " + | |
| indexVar + | |
| "++) {\n" + | |
| branch + | |
| "\n await wait(0);\n" + | |
| "}\n"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@generators/generators-control.js` around lines 177 - 185, The generated loop
uses "for (... in ...)" which can iterate non-numeric keys; change the string
construction in generators-control.js that builds the loop (the block using
variables code, indexVar, listVar and branch) to emit a traditional indexed loop
or a for...of pattern instead—e.g., produce "for (var {indexVar} = 0; {indexVar}
< {listVar}.length; {indexVar}++) { ... await wait(0); }" or emit "for (const
{indexVar} of {listVar}) { ... await wait(0); }" so iteration is over actual
array elements/indices and not enumerable property keys; keep branch and the
await wait(0) inside the loop.
| javascriptGenerator.forBlock["create_custom_map"] = function (block) { | ||
| const colors = []; | ||
| for (let i = 1; i <= 25; i++) { | ||
| const color = | ||
| javascriptGenerator.valueToCode( | ||
| block, | ||
| `COLOR_${i}`, | ||
| javascriptGenerator.ORDER_ATOMIC, | ||
| ) || "#808080"; | ||
| colors.push(color); | ||
| } | ||
| return `await createCustomMap([${colors.join(", ")}]);\n`; | ||
| }; |
There was a problem hiding this comment.
Missing quotes around default color value.
Line 166's default || "#808080" will produce unquoted output like createCustomMap([#808080, ...]) which is invalid JavaScript. The default should be a quoted string.
🔧 Suggested fix
const color =
javascriptGenerator.valueToCode(
block,
`COLOR_${i}`,
javascriptGenerator.ORDER_ATOMIC,
- ) || "#808080";
+ ) || '"#808080"';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@generators/generators-deprecated.js` around lines 158 - 170, The generator
javascriptGenerator.forBlock["create_custom_map"] is using valueToCode(...) ||
"#808080" which produces unquoted token output; change the fallback to a quoted
JS string (e.g. use a fallback like "\"#808080\"" so the generated array
elements are valid string literals), update the code path where COLOR_i is
computed (the valueToCode calls) so defaults include surrounding quotes before
joining and passing into createCustomMap.
| javascriptGenerator.forBlock["get_lexical_variable"] = function (block) { | ||
| const variableName = block.getFieldValue("VAR"); | ||
| const code = variableName; | ||
| return [code, javascriptGenerator.ORDER_ATOMIC]; | ||
| }; |
There was a problem hiding this comment.
get_lexical_variable returns unsanitized field value as code.
The generator returns the raw field value directly as JavaScript code without using nameDB_.getName() for proper variable resolution. This could produce invalid code if the variable name contains special characters or conflicts with reserved words. Consider using proper variable name resolution.
🔧 Suggested fix
javascriptGenerator.forBlock["get_lexical_variable"] = function (block) {
- const variableName = block.getFieldValue("VAR");
- const code = variableName;
+ const variableName = javascriptGenerator.nameDB_.getName(
+ block.getFieldValue("VAR"),
+ Blockly.Names.NameType.VARIABLE,
+ );
+ const code = variableName;
return [code, javascriptGenerator.ORDER_ATOMIC];
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@generators/generators-deprecated.js` around lines 459 - 463, The
get_lexical_variable generator is returning the raw field value from
block.getFieldValue("VAR") instead of resolving/sanitizing it via the name
database; update javascriptGenerator.forBlock["get_lexical_variable"] to call
the name resolution helper (nameDB_.getName with the appropriate variable name
type/constant) on the retrieved variableName and return that resolved identifier
with javascriptGenerator.ORDER_ATOMIC so reserved words/special chars are
handled correctly.
| for (let i = 0; i < block.thenCount_; i++) { | ||
| const thenCode = javascriptGenerator | ||
| .statementToCode(block, "THEN" + i) | ||
| .trim(); | ||
| if (thenCode) { | ||
| thenCodes.push(thenCode); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for thenCount_ definition in block definitions
rg -n "thenCount_" --type jsRepository: flipcomputing/flock
Length of output: 146
🏁 Script executed:
# Search for block definitions - likely in JSON files
fd -e json --type f | head -20 && echo "---" && find . -name "*block*.json" 2>/dev/null | head -20Repository: flipcomputing/flock
Length of output: 223
🏁 Script executed:
# Search for where thenCount_ might be initialized or set
rg "thenCount_\s*=" -nRepository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
# Search for the block definition related to this event block
rg -n "THEN\d+" --type js -B 2 -A 2 | head -50Repository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
# Look for block mutation or initialization functions
rg -n "mutator|onchange|init" generators/generators-events.jsRepository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
# Read the generators-events.js file to understand the block and its mutator
cat -n generators/generators-events.js | head -100Repository: flipcomputing/flock
Length of output: 4286
🏁 Script executed:
# Search for blocks.register or block definitions
rg -n "blocks\.register|defineBlocksWithJsonArray|mutator" -A 5 generators/generators-events.jsRepository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
# Search more broadly for event block definitions and mutators
rg -n "EVENT|event" generators/generators-events.js | head -30Repository: flipcomputing/flock
Length of output: 708
🏁 Script executed:
# Look for block definitions - search for "when_clicked" block definition
rg -n "when_clicked" --type js --type ts -B 5 -A 5 | head -60Repository: flipcomputing/flock
Length of output: 2751
🏁 Script executed:
# Search for block definition files
fd -e ts -e js "block" | grep -v node_modules | head -20Repository: flipcomputing/flock
Length of output: 232
🏁 Script executed:
# Look for files that might contain block definitions
ls -la blocks/ 2>/dev/null || ls -la src/ 2>/dev/null || ls -la . | head -30Repository: flipcomputing/flock
Length of output: 1466
🏁 Script executed:
# Read blocks/events.js to find the when_clicked block definition
cat -n blocks/events.js | head -150Repository: flipcomputing/flock
Length of output: 5396
🏁 Script executed:
# Search for when_clicked in blocks/events.js
rg -n "when_clicked" blocks/events.js -A 20 | head -80Repository: flipcomputing/flock
Length of output: 1648
🏁 Script executed:
# Continue reading blocks/events.js to see the full when_clicked definition
sed -n '91,250p' blocks/events.js | cat -nRepository: flipcomputing/flock
Length of output: 5921
🏁 Script executed:
# Search for thenCount_ initialization in blocks/events.js
rg -n "thenCount_" blocks/events.js -B 3 -A 3Repository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
# Search entire codebase for any assignment to thenCount_
rg "thenCount_\s*=" -nRepository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
# Search for any dynamic property assignment or block manipulation for thenCount_
rg "thenCount" -nRepository: flipcomputing/flock
Length of output: 146
🏁 Script executed:
# Check toolbox.js for when_clicked with extraState that might initialize thenCount_
rg -A 15 "when_clicked" toolbox.js | grep -A 15 "extraState"Repository: flipcomputing/flock
Length of output: 409
Remove or properly initialize block.thenCount_ property.
The loop at line 34 accesses block.thenCount_ which is never initialized in the block definition or any mutation handler. Since thenCount_ is undefined, the condition i < block.thenCount_ evaluates to i < undefined (NaN), causing the loop to never execute and silently skipping all "THEN" statement processing.
Either initialize thenCount_ in the block's init() function (e.g., this.thenCount_ = 0;), add logic to dynamically manage it when THEN inputs are added, or remove this loop entirely if the block design doesn't support multiple THEN inputs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@generators/generators-events.js` around lines 34 - 41, The generator loop
uses block.thenCount_ which is never initialized so the for-loop never runs;
either initialize this property in the block definition (e.g., set
this.thenCount_ = 0 in the block's init() and update it whenever THEN inputs are
added/removed) or stop using it and instead iterate over the actual THEN inputs
dynamically (e.g., detect "THEN0", "THEN1", ... until statementToCode returns
empty). Update the block definition/init() to define thenCount_ or replace the
for (let i = 0; i < block.thenCount_; i++) loop in
generators/generators-events.js to iterate based on existing inputs via
javascriptGenerator.statementToCode(block, "THEN"+i) until no code is returned.
| javascriptGenerator.forBlock["on_collision"] = function (block) { | ||
| const modelName = javascriptGenerator.nameDB_.getName( | ||
| block.getFieldValue("MODEL_VAR"), | ||
| Blockly.Names.NameType.VARIABLE, | ||
| true, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Blockly Names.getName method signature and parameters
💡 Result:
The Blockly Names.getName method is a method of the Names class used in code generators to convert a Blockly entity name (such as a variable or procedure name or ID) to a legal exportable entity name, handling collisions and reserved words. Method signature: getName(nameOrId: string, type: Names.NameType): string Parameters: - nameOrId: string - The Blockly name or ID to convert (can be a user-facing name or internal ID). - type: Names.NameType - The type of name, such as Names.NameType.VARIABLE or Names.NameType.PROCEDURE. Usage example (from generator context): generator.nameDB_.getName(block.getFieldValue('VAR'), Names.NameType.VARIABLE) This is accessed via a code generator instance's nameDB_ property, e.g., Blockly.JavaScript.nameDB_.getName(...). The method ensures the output name is unique and valid for the target language.
Citations:
- 1: https://developers.google.com/blockly/reference/js/blockly.names_class.getname_1_method
- 2: https://developers.google.com/blockly/reference/js/blockly.names_class
- 3: Address nameDB_ or add api to get name of a variable RaspberryPiFoundation/blockly#6008
- 4: https://developers.google.com/blockly/reference/js/blockly.names_namespace.nametype_enum
- 5: Rename Blockly.<language>.variableDB_ to .nameDB_ and add .nameDB_.getUserNames('VARIABLE') and .nameDB_.getUserNames('PROCEDURE') RaspberryPiFoundation/blockly#4845
🏁 Script executed:
# First, let's look at the file in question
cat -n generators/generators-events.js | head -80 | tail -30Repository: flipcomputing/flock
Length of output: 1190
🏁 Script executed:
# Search for other getName() calls to see if the third parameter is used elsewhere
rg "getName\(" --context 2Repository: flipcomputing/flock
Length of output: 50377
🏁 Script executed:
# Check for Blockly version and imports
fd -e json -e js -e ts | xargs grep -l "blockly" | head -5Repository: flipcomputing/flock
Length of output: 146
🏁 Script executed:
# Look for any custom name database implementations
rg "nameDB_" --context 3Repository: flipcomputing/flock
Length of output: 50376
🏁 Script executed:
# Search for any custom implementation of nameDB_ or getName modifications
rg "nameDB_\s*=" --context 3Repository: flipcomputing/flock
Length of output: 50376
🏁 Script executed:
# Look for package.json to find Blockly version
find . -name "package.json" -type f | xargs cat | grep -i blockly -A 2 -B 2Repository: flipcomputing/flock
Length of output: 853
🏁 Script executed:
# Search for any comments explaining the third parameter usage
rg "getName.*true" -B 2 -A 2 --context 3Repository: flipcomputing/flock
Length of output: 50376
🏁 Script executed:
# Search for any custom Names class implementation
rg "class.*Names\|function.*Names" --context 3Repository: flipcomputing/flock
Length of output: 45
Remove the non-standard third argument to getName().
Lines 64-67 and 70-73 pass true as a third argument to javascriptGenerator.nameDB_.getName(), but Blockly's Names.getName() method only accepts two parameters: nameOrId and type. Remove the third argument:
Code snippet
// Before (lines 64-67):
const modelName = javascriptGenerator.nameDB_.getName(
block.getFieldValue("MODEL_VAR"),
Blockly.Names.NameType.VARIABLE,
true,
);
// After:
const modelName = javascriptGenerator.nameDB_.getName(
block.getFieldValue("MODEL_VAR"),
Blockly.Names.NameType.VARIABLE,
);Apply the same fix to otherModelName on lines 70-73, and to similar calls in generators/generators-deprecated.js.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@generators/generators-events.js` around lines 63 - 68, Remove the unsupported
third boolean argument passed to nameDB_.getName() in the collision block
generator: in javascriptGenerator.forBlock["on_collision"] (and the similar
calls creating otherModelName) call javascriptGenerator.nameDB_.getName with
only two arguments (the result of block.getFieldValue("MODEL_VAR") or the other
field and Blockly.Names.NameType.VARIABLE) — do the same cleanup for equivalent
getName calls in generators-deprecated.js so no getName invocation passes a
third parameter.
| const args = []; | ||
| const variables = block.arguments_; | ||
| for (let i = 0; i < variables.length; i++) { |
There was a problem hiding this comment.
Add null-safety for block.arguments_.
Line 70 uses block.arguments_ || [] as a fallback, but line 91 accesses block.arguments_ directly. If arguments_ is undefined, the loop will throw.
Proposed fix
const args = [];
- const variables = block.arguments_;
+ const variables = block.arguments_ || [];
for (let i = 0; i < variables.length; i++) {📝 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.
| const args = []; | |
| const variables = block.arguments_; | |
| for (let i = 0; i < variables.length; i++) { | |
| const args = []; | |
| const variables = block.arguments_ || []; | |
| for (let i = 0; i < variables.length; i++) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@generators/generators-functions.js` around lines 90 - 92, The loop accesses
block.arguments_ directly which can be undefined; change the variables
initialization to be null-safe (e.g., set variables = block.arguments_ || [] or
guard before the loop) so the for loop over variables (used to build args) never
throws when block.arguments_ is missing; update the code paths that reference
variables/args accordingly (look for variables and args in this block to apply
the same null-safety).
| const textColor = getFieldValue(block, "TEXT_COLOR", "#000000"); | ||
| const backgroundColor = getFieldValue(block, "BACKGROUND_COLOR", "#ffffff"); | ||
|
|
||
| const mode = block.getFieldValue("MODE") || ""; | ||
| const asyncMode = block.getFieldValue("ASYNC"); | ||
| const asyncWrapper = asyncMode === "AWAIT" ? "await " : ""; | ||
|
|
||
| const safeTextArg = emitSafeTextArg(textCode); | ||
|
|
||
| return `${asyncWrapper}say(${meshVariable}, { text: ${safeTextArg}, duration: ${durationCode}, textColor: ${textColor}, backgroundColor: ${backgroundColor}, alpha: ${alphaCode}, size: ${sizeCode}, mode: ${JSON.stringify(mode)} });\n`; |
There was a problem hiding this comment.
Missing quotes around color values in say block.
getFieldValue with defaults "#000000" and "#ffffff" returns unquoted strings. When interpolated on line 76, this generates invalid JavaScript like textColor: #000000`` instead of textColor: "#000000".
Compare with line 28 where the default is '"#9932CC"' (with inner quotes) or line 380 where it's '"#FFFFFF"'.
Proposed fix
- const textColor = getFieldValue(block, "TEXT_COLOR", "#000000");
- const backgroundColor = getFieldValue(block, "BACKGROUND_COLOR", "#ffffff");
+ const textColor = getFieldValue(block, "TEXT_COLOR", '"#000000"');
+ const backgroundColor = getFieldValue(block, "BACKGROUND_COLOR", '"#ffffff"');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@generators/generators-text.js` around lines 67 - 76, The generated say(...)
call is inserting raw color strings unquoted (textColor: `#000000`), causing
invalid JS; update the code that builds the return string in generators-text.js
to ensure color values are properly quoted—e.g., replace ${textColor} and
${backgroundColor} with ${JSON.stringify(textColor)} (or otherwise wrap them in
quotes) when constructing the say(...) argument so textColor and backgroundColor
become valid string literals; locate the block where textColor/backgroundColor
are read from getFieldValue and adjust the interpolation in the return string
for say(...) accordingly.
| javascriptGenerator.forBlock["move_by_xyz_single"] = function (block) { | ||
| const modelName = javascriptGenerator.nameDB_.getName( | ||
| block.getFieldValue("BLOCK_NAME"), | ||
| Blockly.Names.NameType.VARIABLE, | ||
| ); | ||
|
|
||
| const coordinate = block.getFieldValue("COORDINATE") || "x_coordinate"; | ||
| const value = getFieldValue(block, "VALUE", "0"); | ||
|
|
||
| switch (coordinate) { | ||
| case "x_coordinate": | ||
| return `await moveByVector(${modelName}, { x: ${value}, y: 0, z: 0 });\n`; | ||
| case "y_coordinate": | ||
| return `await moveByVector(${modelName}, { x: 0, y: ${value}, z: 0 });\n`; | ||
| case "z_coordinate": | ||
| return `await moveByVector(${modelName}, { x: 0, y: 0, z: ${value} });\n`; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Missing default case in switch statement.
The move_by_xyz_single generator's switch statement has no default case. If coordinate has an unexpected value, the function returns undefined, which Blockly will interpret as no code generated. Consider adding a default case that either handles the error or falls back to a sensible default.
🔧 Suggested fix
case "z_coordinate":
return `await moveByVector(${modelName}, { x: 0, y: 0, z: ${value} });\n`;
+ default:
+ return `await moveByVector(${modelName}, { x: ${value}, y: 0, z: 0 });\n`;
}📝 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.
| javascriptGenerator.forBlock["move_by_xyz_single"] = function (block) { | |
| const modelName = javascriptGenerator.nameDB_.getName( | |
| block.getFieldValue("BLOCK_NAME"), | |
| Blockly.Names.NameType.VARIABLE, | |
| ); | |
| const coordinate = block.getFieldValue("COORDINATE") || "x_coordinate"; | |
| const value = getFieldValue(block, "VALUE", "0"); | |
| switch (coordinate) { | |
| case "x_coordinate": | |
| return `await moveByVector(${modelName}, { x: ${value}, y: 0, z: 0 });\n`; | |
| case "y_coordinate": | |
| return `await moveByVector(${modelName}, { x: 0, y: ${value}, z: 0 });\n`; | |
| case "z_coordinate": | |
| return `await moveByVector(${modelName}, { x: 0, y: 0, z: ${value} });\n`; | |
| } | |
| }; | |
| javascriptGenerator.forBlock["move_by_xyz_single"] = function (block) { | |
| const modelName = javascriptGenerator.nameDB_.getName( | |
| block.getFieldValue("BLOCK_NAME"), | |
| Blockly.Names.NameType.VARIABLE, | |
| ); | |
| const coordinate = block.getFieldValue("COORDINATE") || "x_coordinate"; | |
| const value = getFieldValue(block, "VALUE", "0"); | |
| switch (coordinate) { | |
| case "x_coordinate": | |
| return `await moveByVector(${modelName}, { x: ${value}, y: 0, z: 0 });\n`; | |
| case "y_coordinate": | |
| return `await moveByVector(${modelName}, { x: 0, y: ${value}, z: 0 });\n`; | |
| case "z_coordinate": | |
| return `await moveByVector(${modelName}, { x: 0, y: 0, z: ${value} });\n`; | |
| default: | |
| return `await moveByVector(${modelName}, { x: ${value}, y: 0, z: 0 });\n`; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@generators/generators-transform.js` around lines 25 - 42, The switch in
javascriptGenerator.forBlock["move_by_xyz_single"] can return undefined for
unexpected coordinate values; add a default case to the switch on coordinate
(which uses getFieldValue and modelName) that provides a sensible fallback or
explicit error handling—for example, fall back to x (call moveByVector with x:
value, y:0, z:0) or throw/log an error via the same code path—so the function
always returns a valid string instead of undefined.
| const xOrigin = block.getFieldValue("X_ORIGIN") || "'CENTRE'"; | ||
| const yOrigin = block.getFieldValue("Y_ORIGIN") || "'CENTRE'"; | ||
| const zOrigin = block.getFieldValue("Z_ORIGIN") || "'CENTRE'"; | ||
|
|
||
| return `await scale(${modelName}, { x: ${x}, y: ${y}, z: ${z}, xOrigin: '${xOrigin}', yOrigin: '${yOrigin}', zOrigin: '${zOrigin}' });\n`; |
There was a problem hiding this comment.
Default origin values produce double-quoted strings.
The defaults like "'CENTRE'" will produce output like xOrigin: ''CENTRE'' (double single-quotes). The default should either be "CENTRE" (if getFieldValue returns it unquoted) or the quoting in the return statement should be adjusted.
🔧 Suggested fix - assuming field returns unquoted value
- const xOrigin = block.getFieldValue("X_ORIGIN") || "'CENTRE'";
- const yOrigin = block.getFieldValue("Y_ORIGIN") || "'CENTRE'";
- const zOrigin = block.getFieldValue("Z_ORIGIN") || "'CENTRE'";
+ const xOrigin = block.getFieldValue("X_ORIGIN") || "CENTRE";
+ const yOrigin = block.getFieldValue("Y_ORIGIN") || "CENTRE";
+ const zOrigin = block.getFieldValue("Z_ORIGIN") || "CENTRE";📝 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.
| const xOrigin = block.getFieldValue("X_ORIGIN") || "'CENTRE'"; | |
| const yOrigin = block.getFieldValue("Y_ORIGIN") || "'CENTRE'"; | |
| const zOrigin = block.getFieldValue("Z_ORIGIN") || "'CENTRE'"; | |
| return `await scale(${modelName}, { x: ${x}, y: ${y}, z: ${z}, xOrigin: '${xOrigin}', yOrigin: '${yOrigin}', zOrigin: '${zOrigin}' });\n`; | |
| const xOrigin = block.getFieldValue("X_ORIGIN") || "CENTRE"; | |
| const yOrigin = block.getFieldValue("Y_ORIGIN") || "CENTRE"; | |
| const zOrigin = block.getFieldValue("Z_ORIGIN") || "CENTRE"; | |
| return `await scale(${modelName}, { x: ${x}, y: ${y}, z: ${z}, xOrigin: '${xOrigin}', yOrigin: '${yOrigin}', zOrigin: '${zOrigin}' });\n`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@generators/generators-transform.js` around lines 176 - 180, The defaults for
xOrigin/yOrigin/zOrigin are currently set to "'CENTRE'", causing double quotes
when interpolated; change the defaults to unquoted strings (e.g. "CENTRE") by
using xOrigin = block.getFieldValue("X_ORIGIN") || "CENTRE" (same for yOrigin
and zOrigin) and ensure the template interpolation in the return (the scale call
using modelName and xOrigin/yOrigin/zOrigin) does not add extra quotes around
those variables so the generated code becomes xOrigin: 'CENTRE' only once;
update references in this file for xOrigin, yOrigin, zOrigin and the return that
builds the scale(...) call accordingly.
| export function createMesh(block, meshType, params) { | ||
| const { generatedName: variableName, userVariableName } = getVariableInfo( | ||
| block, | ||
| "ID_VAR", | ||
| ); | ||
|
|
||
| const meshId = `${userVariableName}__${block.id}`; | ||
|
|
||
| meshMap[block.id] = block; | ||
| meshBlockIdMap[block.id] = block.id; | ||
|
|
||
| const doCode = block.getInput("DO") | ||
| ? javascriptGenerator.statementToCode(block, "DO") || "" | ||
| : ""; | ||
|
|
||
| const options = [...params]; | ||
|
|
||
| return `${variableName} = create${meshType}("${meshId}", { ${options.join(", ")} });\n${doCode}`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all createMesh calls to verify argument count
rg -n "createMesh\(" --type js -A 1Repository: flipcomputing/flock
Length of output: 50376
🏁 Script executed:
#!/bin/bash
# Find all createMesh calls to verify argument count
rg -n "createMesh\(" --type jsRepository: flipcomputing/flock
Length of output: 50376
Function signature mismatch: createMesh receives 4 arguments but declares only 3 parameters.
All call sites in generators/generators-scene.js (lines 268, 288, 310, 328, 346) pass a fourth argument—a mesh type prefix like "box", "sphere", etc.—but the function signature at line 91 only declares (block, meshType, params). The fourth argument is silently ignored and never used in the function body. Either add the missing parameter to the signature if it was intended for use, or remove it from all call sites if it's unnecessary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@generators/generators-utilities.js` around lines 91 - 109, The createMesh
function currently declares (block, meshType, params) but callers pass a fourth
argument (a mesh type prefix); add a fourth parameter (e.g., meshPrefix) to the
createMesh signature and use it where the mesh identity is built (for example
incorporate meshPrefix into meshId or into the create${meshType} call) so the
passed prefix is not silently ignored; alternatively, if the prefix is truly
unnecessary, remove the extra argument from all call sites in
generators/generators-scene.js. Ensure the change references the createMesh
function and the meshId construction (variableName, userVariableName, block.id)
so callers and behavior remain consistent.
|
None of CodeRabbit's comments relate to the refactor, they are all issues with the generators themselves. |
2 participants
Summary
generators/generators.jsinto multiple filesmesh-state.jsfor code relating to meshes and their stategenerators-utilities.jsfor utility functions used in various placesgenerators-deprecated.jsjust in case there is a mistake. This file and the associated import/function call ingenerators.jscan be removed once you're happy, or kept in case you want to reinstate those blocks in the future.Claude Sonnet 4.6 was used largely to identify which generators were in which section of the toolbox, and as a quick method for auditing each new file to check which utility imports were missing. I also ran my refactoring plan past Claude and asked for comments.
Addresses #220
Summary by CodeRabbit
Release Notes