Merged
Conversation
Made within an interactive session with claude-sonnet 4.6.
Made sure CSG operations consistently return model ids and tests expect model ids rather than meshes throughout. Used an interactive session with claude-sonnet 4.6, but not sure it helped much!
Updated API to allow instant rotation in the case where the duration is 0. Interactive debugging and fixing session assisted by claude-sonnet 4.6
Sound tests checking for properties that don't exist in the config. Interactive session assisted by claude-sonnet 4.6
Spotted by coderabbit review and suggested fix applied.
Added tests for printText API method in interactive session with claude-sonnet 4.6, plan first, review approach and then generate.
Added tests for the UISlider and UIInput compnents. Interactive session with plan, review and action claude-sonnet 4.6
Removed old duplicate implementation of button controls. Tests for buttons controls Interactive session with planning, review and implementation with claude-sonnet 4.6
Guided claude-sonnet 4.6 to create a plan to add missing sky tests. Iterarated to add missing cases including different material options. Then implemented and reviewed.
Added tests for events api Interactive planning and implementation with claude-sonnet 4.6. Agreeing cases first and identifying missing coverage and debugging.
Reviewed missing physics test. Worked in an interactive session with claude-sonnet 4.6 to create and update a plan and then implement.
Interactive session with claude-sonnet 4.6. Created a plan and reviewed then generated implementaion and updated based on coderabbit feedback.
…topFollow, hold, attach, and drop Tests use Liz3.glb + tree.glb with NullEngine for the bone-attachment tests. attach/drop tests verify behavioral correctness: attached mesh follows Liz when she moves; dropped mesh stays put after Liz moves. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eSystem, stopParticleSystem, resetParticleSystem Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pply CodeRabbit fixes - createCapsule, createPlane, create3DText tests in new shapes.test.js - create3DText tagged @slow (fetches and processes a font file) - Fix mesh-hierarchy parentChild offset test to assert childMesh.position.x/z - Restore flock.mainLight after mock in lightColor test - Error-resilient dispose in afterEach for shapes and mesh-hierarchy tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tests assert BPM is stored on scene/mesh metadata and clamps invalid values to 60. speak tests use a timeout race to handle the headless speechSynthesis environment where onvoiceschanged never fires. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tests assert key bindings are stored and deduplicated in _cameraControlBindings, that attachCamera sets activeCamera with the correct following metadata, and that canvasControls does not throw. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rotateToObject: assert mesh rotation changes after facing a mesh at a different position (duration 0 for speed). positionAtSingleCoordinate: assert x/z via mesh.position and y via minimumWorld.y since positionAt aligns the base of the mesh to the target Y. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
No implementation existed; the create_ground UI case is superseded by create_map. Removes the binding from flock.js and the dead case from ui/addmeshes.js. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
waitUntil tests pump the scene render loop so onBeforeRenderObservable fires. safeLoop tests verify the body is called per iteration and that stopExecution halts early. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Mode controllerRumble, controllerRumblePattern, playRumblePattern and the rumblePatterns table are removed — not ready to be exposed in the API. Tests stub initializeXR, printText, and window.translate so the XR smoke tests run cleanly in headless Chromium without WebXR hardware. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers: returns string ID, mesh exists after load, correct modelName metadata, position, and show/hide. Uses NullEngine + physics mock pattern from characterAnimations tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
initialize: stops render loop, calls flock.initialize(), verifies BABYLON, canvas, AbortController, and key observables, then restores scene and render loop. createEngine: swaps in a throwaway NullEngine, calls createEngine(), verifies a real Engine is created with enableOfflineSupport false, then restores the original engine and scene. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR refactors animation handling for instant rotations, removes haptic/rumble and ground-creation APIs, simplifies UI styling defaults, migrates test infrastructure to use IP addresses instead of localhost, and adds comprehensive test coverage across 14+ new test modules spanning animation, physics, events, sensing, and various scene/UI APIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
api/csg.js (1)
305-315:⚠️ Potential issue | 🟠 MajorThe single-mesh merge now leaves two result candidates under the same id.
Line 315 returns only
modelId, but this branch still keeps bothsingleMeshand its clone in the scene with that same name. The new id-based lookup path intests/materials.test.jsno longer has a stable way to identify which one is the merged result, and the extra mesh is leaked.Proposed fix
if (meshesToMerge.length === 1) { const singleMesh = meshesToMerge[0]; let mergedMesh = singleMesh.clone(modelId); if (!mergedMesh) mergedMesh = singleMesh; + if (mergedMesh !== singleMesh) { + singleMesh.dispose(); + } mergedMesh.name = modelId; mergedMesh.metadata = mergedMesh.metadata || {}; mergedMesh.metadata.blockKey = blockId; mergedMesh.metadata.sharedMaterial = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/csg.js` around lines 305 - 315, The branch handling meshesToMerge.length === 1 clones singleMesh into mergedMesh but leaves both objects under the same name/id, causing duplicate candidates; update the logic in the meshesToMerge === 1 branch (referencing meshesToMerge, singleMesh, mergedMesh, modelId, blockId) so that after cloning you either replace the original in the scene or remove the original singleMesh and ensure only mergedMesh remains with name/modelId and metadata.blockKey = blockId, then return the stable identifier for the merged mesh (the retained mergedMesh/modelId) so id-based lookups no longer see a leaked duplicate.tests/materials.test.js (2)
857-869:⚠️ Potential issue | 🟡 MinorRestore
console.warnin afinallyblock.If
mergeMeshes()rejects here, the patched global survives into later tests and contaminates the suite.Proposed fix
const warningMessages = []; const originalWarn = console.warn; - console.warn = (...args) => { - warningMessages.push(args.join(" ")); - return originalWarn(...args); - }; - - const id = await flock.mergeMeshes("mergeWithInvalid", [ - "mergeGoodBox", - "mergeInvalidMesh", - ]); - console.warn = originalWarn; + let id; + try { + console.warn = (...args) => { + warningMessages.push(args.join(" ")); + return originalWarn(...args); + }; + + id = await flock.mergeMeshes("mergeWithInvalid", [ + "mergeGoodBox", + "mergeInvalidMesh", + ]); + } finally { + console.warn = originalWarn; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/materials.test.js` around lines 857 - 869, The test temporarily overrides console.warn (originalWarn) to capture messages before calling flock.mergeMeshes("mergeWithInvalid", [...]) but restores console.warn only after the await, so a rejection would leak the mock; wrap the restore in a try/finally (or move the restoration into a finally block) so console.warn is set back to originalWarn regardless of whether mergeMeshes resolves or rejects, and ensure meshIds.push("mergeWithInvalid") remains only when appropriate (e.g., after a successful merge) or is moved out of the finally if it should not run on failure.
566-575:⚠️ Potential issue | 🟠 MajorThese updated CSG tests still aren't awaiting the operation they're asserting on.
The new id-based calls are fired inside nested
whenModelReady(...)callbacks, but nothing isawaited or returned from those callback chains. That means Mocha can finish the test before the finalexpect(...)runs, so these cases can false-pass.Representative rewrite
- flock.whenModelReady("box1", (box1) => { - flock.whenModelReady("box2", (box2) => { - const materialsBefore = flock.scene.materials.length; - flock.subtractMeshes("subtracted", "box1", ["box2"]); - boxIds.push("subtracted"); - flock.whenModelReady("subtracted", (mesh) => { - expect(flock.scene.materials.length).to.equal(materialsBefore - 1); - }); - }); - }); + const materialsBefore = flock.scene.materials.length; + const id = await flock.subtractMeshes("subtracted", "box1", ["box2"]); + boxIds.push(id); + const mesh = flock.scene.getMeshByName(id); + expect(mesh).to.exist; + expect(flock.scene.materials.length).to.equal(materialsBefore - 1);Also applies to: 595-603, 624-632, 653-660, 681-688, 709-715, 737-743
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/materials.test.js` around lines 566 - 575, The nested whenModelReady(...) callbacks in the tests (e.g., around subtractMeshes("subtracted", "box1", ["box2"])) are not awaited or returned, allowing Mocha to finish before the final expect runs; change each test to return or await the async completion of the whenModelReady chain—for example make the outer test async and wrap the whenModelReady callback(s) in Promises (or modify whenModelReady to return a Promise) so you can await the inner whenModelReady that performs the expect; ensure you await the call that triggers the CSG operation (subtractMeshes, addMeshes, intersectMeshes, etc.) and then await the whenModelReady for the resulting id before asserting.tests/objects.test.js (1)
151-202:⚠️ Potential issue | 🟠 MajorOrphaned test case — missing
it(prefix.This test is defined as a bare string and function expression without the
it(call, so it will never be executed by Mocha.🐛 Proposed fix
- ("should correctly handle modelId and blockId splitting", + it("should correctly handle modelId and blockId splitting", function () {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/objects.test.js` around lines 151 - 202, The test block starting with the bare string should be turned into a valid Mocha test by wrapping it with it(...): replace the orphaned ("should correctly handle modelId and blockId splitting", function () { ... }); with it("should correctly handle modelId and blockId splitting", function () { ... }); so the suite will execute; keep all inner assertions and usages of flock.createObject and flock.scene.getMeshByName unchanged and ensure the final closing parentheses/bracket from the original block remain properly balanced.
🧹 Nitpick comments (9)
tests/transform.translate.test.js (3)
422-430: MissingsetAnchorcall unlike other test suites.Other test blocks in this file (e.g., lines 19, 129-133) call
await flock.setAnchor(boxId)after creating the box to establish consistent pivot settings. This is particularly important for the y-coordinate test since the API's bounding-box offset behavior depends on the anchor/pivot configuration.Consider adding:
await flock.createBox(boxId, { width: 1, height: 1, depth: 1, position: [0, 0, 0], }); + await flock.setAnchor(boxId); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transform.translate.test.js` around lines 422 - 430, The beforeEach hook creates a box with flock.createBox but does not set the pivot/anchor like other tests; after creating the box (in the same beforeEach) call await flock.setAnchor(boxId) to match the other suites and ensure consistent bounding-box offset behavior for the y-coordinate tests—update the beforeEach to invoke flock.setAnchor(boxId) immediately after flock.createBox(...) where boxId is defined.
440-444: Tests don't verify that other coordinates remain unchanged.Each test sets one coordinate but doesn't assert that the other two coordinates are unaffected. This could mask bugs where setting one coordinate inadvertently modifies another.
♻️ Suggested improvement for more complete assertions
it("should set only the x coordinate", async function () { + const mesh = flock.scene.getMeshByName(boxId); + const initialY = mesh.position.y; + const initialZ = mesh.position.z; + await flock.positionAtSingleCoordinate(boxId, "x_coordinate", 7); - const mesh = flock.scene.getMeshByName(boxId); expect(mesh.position.x).to.be.closeTo(7, 0.01); + expect(mesh.position.y).to.be.closeTo(initialY, 0.01); + expect(mesh.position.z).to.be.closeTo(initialZ, 0.01); });Apply similar pattern to the z-coordinate test. The y-coordinate test is more complex due to bounding-box offset semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transform.translate.test.js` around lines 440 - 444, The test for setting a single coordinate should also assert the other two coordinates are unchanged: before calling positionAtSingleCoordinate(boxId, "x_coordinate", 7) capture the original mesh via flock.scene.getMeshByName(boxId) and store originalPos = {x: mesh.position.x, y: mesh.position.y, z: mesh.position.z}; after the call assert mesh.position.x is closeTo(7, 0.01) and assert mesh.position.y and mesh.position.z are still closeTo originalPos.y and originalPos.z (use the same tolerance); apply the same pattern to the z-coordinate test (for y-coordinate test keep current handling due to bounding-box offset semantics).
432-438: Try/catch in afterEach is inconsistent with other test suites.Other
afterEachblocks in this file (lines 22-26, 136-138, 197-200) do not wrapdisposein try/catch. While defensive, this inconsistency could mask real cleanup failures in development. Consider aligning with the existing pattern or applying the try/catch approach consistently across all test suites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transform.translate.test.js` around lines 432 - 438, The afterEach block wraps flock.dispose(boxId) in a try/catch, which is inconsistent with other suites and can hide cleanup failures; update this block to match the existing pattern by removing the try/catch and calling flock.dispose(boxId) directly in the afterEach (keep the same afterEach function and boxId usage), or alternatively apply the same try/catch pattern to the other afterEach blocks—preferably remove the try/catch here so the cleanup failure surfaces during tests.api/animate.js (1)
163-169: Consider addingphysicsType !== "NONE"check for consistency with transform.js.The instant path checks
mesh.physics && mesh.physics._pluginData?.hpBodyIdbut omits themesh.metadata?.physicsType !== "NONE"check that the similar instant teleport code intransform.js(lines 125-140) uses. This could lead tosetTargetTransformbeing called on meshes where physics is logically disabled via metadata.♻️ Suggested fix for consistency
- if (mesh.physics && mesh.physics._pluginData?.hpBodyId) { + if ( + mesh.physics && + mesh.metadata?.physicsType !== "NONE" && + mesh.physics._pluginData?.hpBodyId + ) { mesh.physics.setTargetTransform( mesh.absolutePosition, mesh.absoluteRotationQuaternion || flock.BABYLON.Quaternion.FromEulerVector(mesh.rotation), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/animate.js` around lines 163 - 169, The current conditional calls mesh.physics.setTargetTransform when mesh.physics && mesh.physics._pluginData?.hpBodyId is true but misses the metadata flag check; update the condition to also require mesh.metadata?.physicsType !== "NONE" (i.e., only call setTargetTransform if physics exists, hpBodyId is present, and metadata.physicsType is not "NONE") so it matches the behavior used in the instant teleport code (see uses of mesh.metadata?.physicsType in transform.js) and prevents calling setTargetTransform for meshes with physics disabled.tests/animate.test.js (1)
963-987: Test covers the instant rotation path, but assertion could be stronger.The test verifies that
rotation.ychanged, but doesn't verify it changed to the expected value. With mesh2 at[5, 0, 0]and mesh1 at[0, 0, 0], mesh1 should rotate to face the positive X direction. Consider asserting the expected angle for more confidence:🧪 Suggested stronger assertion
await flock.rotateToObject(id1, id2, { duration: 0 }); - expect(mesh1.rotation.y).to.not.be.closeTo(initialRotationY, 0.001); + // mesh1 at origin facing mesh2 at [5,0,0] should rotate ~-90° around Y + expect(mesh1.rotation.y).to.be.closeTo(-Math.PI / 2, 0.1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/animate.test.js` around lines 963 - 987, The test only checks that mesh1.rotation.y changed after flock.rotateToObject but not that it rotated to the correct bearing; update the assertion to verify mesh1 faces the positive X axis by asserting mesh1.rotation.y is approximately Math.PI/2 (use a small tolerance like 0.001), referencing the existing symbols rotateToObject, flock.createBox, flock.scene.getMeshByName, and mesh1.rotation.y so the test asserts the expected angle rather than just any change.tests/effects.test.js (1)
226-240: Consider using public API instead of internal_stoppedproperty.The test relies on
ps._stopped, which is an internal BabylonJS implementation detail (underscore prefix). This could break if BabylonJS changes its internals. Consider using the publicisStarted()method for more stable assertions:ps.start(); - expect(ps._stopped).to.be.false; + expect(ps.isStarted()).to.be.true; flock.stopParticleSystem("stopTestPS"); - expect(ps._stopped).to.be.true; + expect(ps.isStarted()).to.be.false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/effects.test.js` around lines 226 - 240, The test currently inspects the internal BabylonJS flag ps._stopped; instead, use the public ParticleSystem API: after ps.start() assert ps.isStarted() (or expect(ps.isStarted()).to.be.true) and after calling flock.stopParticleSystem("stopTestPS") assert that ps.isStarted() is false (or expect(ps.isStarted()).to.be.false). Update the test that references ps._stopped to use ps.isStarted() and keep the rest (createdEffects, start/stop calls, and the flock.stopParticleSystem invocation) unchanged.tests/physics.test.js (1)
345-358: Test asserts logging occurred but doesn't verify the log message content.The test only checks that
console.logwas called, but doesn't verify it's the expected "mesh not found" message. Consider checking for a specific substring to avoid false positives from unrelated logs.♻️ Optional improvement
it("should log when mesh not found", function () { - let logged = false; + let loggedNotFound = false; const originalConsoleLog = console.log; console.log = (...args) => { - logged = true; + if (args.some(arg => String(arg).includes("nonExistentMesh"))) { + loggedNotFound = true; + } originalConsoleLog(...args); }; flock.up("nonExistentMesh", 5); console.log = originalConsoleLog; - expect(logged).to.be.true; + expect(loggedNotFound).to.be.true; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/physics.test.js` around lines 345 - 358, The test currently only asserts that console.log was called when calling flock.up("nonExistentMesh", 5) but doesn't verify the message; update the test to capture the console output (the existing override of console.log using originalConsoleLog and logged) and assert that the joined args contain the expected substring like "mesh not found" (or the exact message emitted by flock.up) instead of just setting logged = true; ensure you still restore console.log (use a try/finally or after hook) and consider using the existing originalConsoleLog variable or a Sinon spy if available to assert the specific message content.tests/objects.test.js (1)
3-27: Consider extracting shared test utilities.
configureDracoandpumpAnimationare duplicated here and inmesh-hierarchy.test.js. Consider extracting to a shared test utilities module to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/objects.test.js` around lines 3 - 27, The functions configureDraco and pumpAnimation are duplicated across tests; extract them into a shared test utilities module (e.g., export configureDraco and pumpAnimation from a new test-utils file) and import those functions into tests (including objects.test.js and mesh-hierarchy.test.js) to remove duplication; ensure the exported names match the existing functions exactly and update test files to replace the local definitions with imports, keeping behavior (DefaultNumWorkers setting, decoder URLs, and the interval+try/finally/clearInterval logic) unchanged.tests/scene.test.js (1)
394-414: Avoid the fixed 500ms sleep in this async assertion.Using wall-clock delay here makes the test slower and timing-dependent under CI load. Await the clone callback directly instead of sleeping and checking a flag afterward.
Possible fix
- let called = false; - const cloneId = flock.cloneMesh({ - sourceMeshName: boxId, - cloneId: "myClone", - callback: () => { - called = true; - }, - }); - createdIds.push(cloneId); - await new Promise((resolve) => setTimeout(resolve, 500)); - expect(called).to.be.true; + await new Promise((resolve, reject) => { + const timeoutId = setTimeout( + () => reject(new Error("clone callback did not fire")), + 2500, + ); + const cloneId = flock.cloneMesh({ + sourceMeshName: boxId, + cloneId: "myClone", + callback: () => { + clearTimeout(timeoutId); + resolve(); + }, + }); + createdIds.push(cloneId); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scene.test.js` around lines 394 - 414, Replace the fixed 500ms sleep with waiting on the clone callback by turning the test into an async flow that awaits a Promise resolved by the callback passed to flock.cloneMesh; specifically, in the test wrap flock.cloneMesh (or the callback argument you pass into cloneMesh) with new Promise(resolve => { callback: () => { called = true; resolve(); } }) and await that Promise instead of setTimeout, referencing the cloneMesh call and its callback to locate where to change the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/buttoncontrols.test.js`:
- Around line 5-10: The beforeEach uses "??=" which preserves shared state
between tests; instead always replace with fresh per-test instances by assigning
flock.canvas = {} , flock.canvas.pressedButtons = new Set(), flock.displayScale
= 1 and creating new observables for flock.gridKeyPressObservable and
flock.gridKeyReleaseObservable (new flock.BABYLON.Observable()) inside the
beforeEach so tests get a clean slate; update the beforeEach block referencing
the existing function name and these symbols (beforeEach, flock.canvas,
pressedButtons, displayScale, gridKeyPressObservable, gridKeyReleaseObservable,
flock.BABYLON.Observable) to perform direct assignments rather than nullish
coalescing.
In `@tests/effects.test.js`:
- Around line 242-254: The test for resetParticleSystem currently checks only
empty internals; modify it to ensure the ParticleSystem actually contains
particles before calling flock.resetParticleSystem("resetTestPS"): create the
ParticleSystem instance (flock.BABYLON.ParticleSystem "resetTestPS"), configure
emitter and set options, then trigger particle emission (e.g., call the public
emit/start method or directly populate ps._particles and ps._stockParticles with
mock particle objects) so that ps._particles and ps._stockParticles are
non-empty, call flock.resetParticleSystem("resetTestPS"), and assert both arrays
are emptied; also add a short comment noting the test relies on internal
properties (_particles/_stockParticles) if no public API exists.
In `@tests/events.test.js`:
- Around line 359-371: Test currently only asserts mesh2 does not fire; add a
positive control to verify the registration worked by triggering mesh1 and
asserting the counter increments. After calling flock.onTrigger(id1, { trigger:
"OnPickTrigger", callback: () => count++, applyToGroup: false }), call
mesh1.actionManager?.processTrigger(flock.BABYLON.ActionManager.OnPickTrigger)
and assert expect(count).to.equal(1) before running the mesh2 negative assertion
(which should then assert count remains 1 or equals 1 after mesh2 triggers).
- Around line 38-50: The test expectations mismatch the current validator
contract in isAllowedEventName: update tests in tests/events.test.js to reflect
that isAllowedEventName only rejects non-strings, names longer than 30 chars,
and names starting with the reserved prefixes
babylon|pointer|keys|mouse|touch|camera|mic|scene; change the "returns false for
reserved prefixes" block to assert true for names like
"onSomething","systemEvent","internalMsg","flockReady","_hidden" (or remove that
block), and instead add/keep assertions that names with the actual reserved
prefixes (e.g. "babylonTick","pointerMove") return false and that overly long
strings and non-string inputs return false while punctuation (e.g.
"hello!","say@world") remains allowed.
In `@tests/objects.test.js`:
- Around line 359-362: The test is asserting the wrong metadata field; change
the assertion in the test (the it block referencing mesh, meshId, and modelName)
to check mesh.metadata?.templateTag instead of mesh.metadata?.modelName so it
matches the implementation in api/models.js which sets metadata.templateTag;
ensure the expectation compares templateTag to modelName.
In `@tests/physics.test.js`:
- Around line 478-492: The test "should set physicsShapeType to CAPSULE on the
mesh metadata" calls the async factory function flock.createBox without awaiting
it, which can cause a race with the subsequent flock.setPhysicsShape call;
update the test to await flock.createBox(id, {...}) before pushing id into
boxIds and before calling flock.setPhysicsShape so the mesh exists (refer to
flock.createBox and flock.setPhysicsShape in the test).
- Around line 494-508: The test is missing an await when calling flock.createBox
which can cause a race before setPhysicsShape runs; update the test to await
flock.createBox(id, {...}) and only push id into boxIds after the awaited
creation completes so that subsequent calls (flock.setPhysicsShape and
flock.scene.getMeshByName) operate on the fully-created mesh (refer to
createBox, setPhysicsShape, getMeshByName, and mesh.metadata.physicsShapeType).
In `@tests/scene.test.js`:
- Around line 280-290: The test currently only saves/restores flock.scene but
initialize() also mutates flock.engine, flock.canvas, observables,
abortController and the render loop; update the fixture to snapshot the full
pre-initialize state (e.g. save savedEngine, savedCanvas, savedScene,
savedAbortController and any observable references) in before and restore each
in after, ensuring you stop the render loop with flock.engine?.stopRenderLoop()
before the test and restart the original with
flock.engine?.runRenderLoop(flock._renderLoop) after restoring the original
engine and render loop; reference symbols: flock.initialize(), flock.engine,
flock.canvas, flock.scene, flock.abortController, flock._renderLoop and any
observable properties you saved.
In `@tests/sound2.test.js`:
- Around line 22-36: The test calls the async helper flock.createBox without
awaiting it, so setBPM and the mesh lookup may run before the box exists; update
the test to await flock.createBox(id, { ... }) inside the async it block (i.e.,
add await before the flock.createBox call in the test that uses setBPM, then
keep the rest: await flock.setBPM(id, 90), const mesh =
flock.scene.getMeshByName(id), expect(mesh.metadata.bpm).to.equal(90),
flock.dispose(id)).
- Around line 52-66: The test calls flock.createBox without awaiting it, which
can cause race conditions before calling speakWithTimeout and flock.speak; add
await before flock.createBox(id, {...}) so the box is fully created before
invoking speakWithTimeout(() => flock.speak(id, "hello")), and ensure
flock.dispose(id) remains in the finally block; apply the same fix for any other
tests using flock.createBox without await.
---
Outside diff comments:
In `@api/csg.js`:
- Around line 305-315: The branch handling meshesToMerge.length === 1 clones
singleMesh into mergedMesh but leaves both objects under the same name/id,
causing duplicate candidates; update the logic in the meshesToMerge === 1 branch
(referencing meshesToMerge, singleMesh, mergedMesh, modelId, blockId) so that
after cloning you either replace the original in the scene or remove the
original singleMesh and ensure only mergedMesh remains with name/modelId and
metadata.blockKey = blockId, then return the stable identifier for the merged
mesh (the retained mergedMesh/modelId) so id-based lookups no longer see a
leaked duplicate.
In `@tests/materials.test.js`:
- Around line 857-869: The test temporarily overrides console.warn
(originalWarn) to capture messages before calling
flock.mergeMeshes("mergeWithInvalid", [...]) but restores console.warn only
after the await, so a rejection would leak the mock; wrap the restore in a
try/finally (or move the restoration into a finally block) so console.warn is
set back to originalWarn regardless of whether mergeMeshes resolves or rejects,
and ensure meshIds.push("mergeWithInvalid") remains only when appropriate (e.g.,
after a successful merge) or is moved out of the finally if it should not run on
failure.
- Around line 566-575: The nested whenModelReady(...) callbacks in the tests
(e.g., around subtractMeshes("subtracted", "box1", ["box2"])) are not awaited or
returned, allowing Mocha to finish before the final expect runs; change each
test to return or await the async completion of the whenModelReady chain—for
example make the outer test async and wrap the whenModelReady callback(s) in
Promises (or modify whenModelReady to return a Promise) so you can await the
inner whenModelReady that performs the expect; ensure you await the call that
triggers the CSG operation (subtractMeshes, addMeshes, intersectMeshes, etc.)
and then await the whenModelReady for the resulting id before asserting.
In `@tests/objects.test.js`:
- Around line 151-202: The test block starting with the bare string should be
turned into a valid Mocha test by wrapping it with it(...): replace the orphaned
("should correctly handle modelId and blockId splitting", function () { ... });
with it("should correctly handle modelId and blockId splitting", function () {
... }); so the suite will execute; keep all inner assertions and usages of
flock.createObject and flock.scene.getMeshByName unchanged and ensure the final
closing parentheses/bracket from the original block remain properly balanced.
---
Nitpick comments:
In `@api/animate.js`:
- Around line 163-169: The current conditional calls
mesh.physics.setTargetTransform when mesh.physics &&
mesh.physics._pluginData?.hpBodyId is true but misses the metadata flag check;
update the condition to also require mesh.metadata?.physicsType !== "NONE"
(i.e., only call setTargetTransform if physics exists, hpBodyId is present, and
metadata.physicsType is not "NONE") so it matches the behavior used in the
instant teleport code (see uses of mesh.metadata?.physicsType in transform.js)
and prevents calling setTargetTransform for meshes with physics disabled.
In `@tests/animate.test.js`:
- Around line 963-987: The test only checks that mesh1.rotation.y changed after
flock.rotateToObject but not that it rotated to the correct bearing; update the
assertion to verify mesh1 faces the positive X axis by asserting
mesh1.rotation.y is approximately Math.PI/2 (use a small tolerance like 0.001),
referencing the existing symbols rotateToObject, flock.createBox,
flock.scene.getMeshByName, and mesh1.rotation.y so the test asserts the expected
angle rather than just any change.
In `@tests/effects.test.js`:
- Around line 226-240: The test currently inspects the internal BabylonJS flag
ps._stopped; instead, use the public ParticleSystem API: after ps.start() assert
ps.isStarted() (or expect(ps.isStarted()).to.be.true) and after calling
flock.stopParticleSystem("stopTestPS") assert that ps.isStarted() is false (or
expect(ps.isStarted()).to.be.false). Update the test that references ps._stopped
to use ps.isStarted() and keep the rest (createdEffects, start/stop calls, and
the flock.stopParticleSystem invocation) unchanged.
In `@tests/objects.test.js`:
- Around line 3-27: The functions configureDraco and pumpAnimation are
duplicated across tests; extract them into a shared test utilities module (e.g.,
export configureDraco and pumpAnimation from a new test-utils file) and import
those functions into tests (including objects.test.js and
mesh-hierarchy.test.js) to remove duplication; ensure the exported names match
the existing functions exactly and update test files to replace the local
definitions with imports, keeping behavior (DefaultNumWorkers setting, decoder
URLs, and the interval+try/finally/clearInterval logic) unchanged.
In `@tests/physics.test.js`:
- Around line 345-358: The test currently only asserts that console.log was
called when calling flock.up("nonExistentMesh", 5) but doesn't verify the
message; update the test to capture the console output (the existing override of
console.log using originalConsoleLog and logged) and assert that the joined args
contain the expected substring like "mesh not found" (or the exact message
emitted by flock.up) instead of just setting logged = true; ensure you still
restore console.log (use a try/finally or after hook) and consider using the
existing originalConsoleLog variable or a Sinon spy if available to assert the
specific message content.
In `@tests/scene.test.js`:
- Around line 394-414: Replace the fixed 500ms sleep with waiting on the clone
callback by turning the test into an async flow that awaits a Promise resolved
by the callback passed to flock.cloneMesh; specifically, in the test wrap
flock.cloneMesh (or the callback argument you pass into cloneMesh) with new
Promise(resolve => { callback: () => { called = true; resolve(); } }) and await
that Promise instead of setTimeout, referencing the cloneMesh call and its
callback to locate where to change the test.
In `@tests/transform.translate.test.js`:
- Around line 422-430: The beforeEach hook creates a box with flock.createBox
but does not set the pivot/anchor like other tests; after creating the box (in
the same beforeEach) call await flock.setAnchor(boxId) to match the other suites
and ensure consistent bounding-box offset behavior for the y-coordinate
tests—update the beforeEach to invoke flock.setAnchor(boxId) immediately after
flock.createBox(...) where boxId is defined.
- Around line 440-444: The test for setting a single coordinate should also
assert the other two coordinates are unchanged: before calling
positionAtSingleCoordinate(boxId, "x_coordinate", 7) capture the original mesh
via flock.scene.getMeshByName(boxId) and store originalPos = {x:
mesh.position.x, y: mesh.position.y, z: mesh.position.z}; after the call assert
mesh.position.x is closeTo(7, 0.01) and assert mesh.position.y and
mesh.position.z are still closeTo originalPos.y and originalPos.z (use the same
tolerance); apply the same pattern to the z-coordinate test (for y-coordinate
test keep current handling due to bounding-box offset semantics).
- Around line 432-438: The afterEach block wraps flock.dispose(boxId) in a
try/catch, which is inconsistent with other suites and can hide cleanup
failures; update this block to match the existing pattern by removing the
try/catch and calling flock.dispose(boxId) directly in the afterEach (keep the
same afterEach function and boxId usage), or alternatively apply the same
try/catch pattern to the other afterEach blocks—preferably remove the try/catch
here so the cleanup failure surfaces during tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12e58f66-c981-4079-8031-be4055fc556b
⛔ Files ignored due to path filters (1)
tests/textures/Islands.pngis excluded by!**/*.png
📒 Files selected for processing (31)
api/animate.jsapi/csg.jsapi/ui.jsapi/xr.jsflock.jsplaywright.config.jsscripts/run-api-tests.mjstests/animate.test.jstests/buttoncontrols.test.jstests/camera.test.jstests/control.test.jstests/effects.test.jstests/events.test.jstests/materials.test.jstests/math.test.jstests/mesh-hierarchy.test.jstests/models/Flock.glbtests/movement.test.jstests/objects.test.jstests/physics.test.jstests/printtext.test.jstests/scene.test.jstests/sensing.test.jstests/shapes.test.jstests/sound-verification.test.jstests/sound2.test.jstests/tests.htmltests/transform.translate.test.jstests/uitextbutton.test.jstests/xr.test.jsui/addmeshes.js
💤 Files with no reviewable changes (4)
- ui/addmeshes.js
- api/ui.js
- api/xr.js
- flock.js
Comment on lines
+5
to
+10
| beforeEach(function () { | ||
| flock.canvas ??= {}; | ||
| flock.canvas.pressedButtons ??= new Set(); | ||
| flock.displayScale ??= 1; | ||
| flock.gridKeyPressObservable ??= new flock.BABYLON.Observable(); | ||
| flock.gridKeyReleaseObservable ??= new flock.BABYLON.Observable(); |
There was a problem hiding this comment.
Use fresh per-test state here.
These cases rely on displayScale === 1, an empty pressedButtons, and clean observables, but ??= reuses whatever a previous suite left on the shared flock object. That makes the size assertions and observer-driven behavior order-dependent.
Possible fix
beforeEach(function () {
flock.canvas ??= {};
- flock.canvas.pressedButtons ??= new Set();
- flock.displayScale ??= 1;
- flock.gridKeyPressObservable ??= new flock.BABYLON.Observable();
- flock.gridKeyReleaseObservable ??= new flock.BABYLON.Observable();
+ flock.canvas.pressedButtons = new Set();
+ flock.displayScale = 1;
+ flock.gridKeyPressObservable = new flock.BABYLON.Observable();
+ flock.gridKeyReleaseObservable = new flock.BABYLON.Observable();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/buttoncontrols.test.js` around lines 5 - 10, The beforeEach uses "??="
which preserves shared state between tests; instead always replace with fresh
per-test instances by assigning flock.canvas = {} , flock.canvas.pressedButtons
= new Set(), flock.displayScale = 1 and creating new observables for
flock.gridKeyPressObservable and flock.gridKeyReleaseObservable (new
flock.BABYLON.Observable()) inside the beforeEach so tests get a clean slate;
update the beforeEach block referencing the existing function name and these
symbols (beforeEach, flock.canvas, pressedButtons, displayScale,
gridKeyPressObservable, gridKeyReleaseObservable, flock.BABYLON.Observable) to
perform direct assignments rather than nullish coalescing.
Comment on lines
+242
to
+254
| it("resetParticleSystem should clear all particles", function () { | ||
| const ps = new flock.BABYLON.ParticleSystem( | ||
| "resetTestPS", | ||
| 100, | ||
| flock.scene, | ||
| ); | ||
| ps.emitter = flock.BABYLON.Vector3.Zero(); | ||
| createdEffects.push("resetTestPS"); | ||
|
|
||
| flock.resetParticleSystem("resetTestPS"); | ||
| expect(ps._particles).to.have.lengthOf(0); | ||
| expect(ps._stockParticles).to.have.lengthOf(0); | ||
| }); |
There was a problem hiding this comment.
Test doesn't verify clearing behavior on a system with particles.
A freshly created ParticleSystem already has empty _particles and _stockParticles arrays. This test only proves that empty arrays remain empty after reset—it doesn't actually verify that resetParticleSystem clears anything.
To properly test the "should clear all particles" behavior, the particle system should emit particles before reset:
ps.emitter = flock.BABYLON.Vector3.Zero();
createdEffects.push("resetTestPS");
+ ps.start();
+ // Allow some particles to be generated
+ await new Promise((resolve) => setTimeout(resolve, 50));
+ ps.stop();
+
flock.resetParticleSystem("resetTestPS");
expect(ps._particles).to.have.lengthOf(0);
expect(ps._stockParticles).to.have.lengthOf(0);Additionally, relying on internal _particles/_stockParticles properties is fragile. If there's no public API to verify particle count, document this dependency on internals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/effects.test.js` around lines 242 - 254, The test for
resetParticleSystem currently checks only empty internals; modify it to ensure
the ParticleSystem actually contains particles before calling
flock.resetParticleSystem("resetTestPS"): create the ParticleSystem instance
(flock.BABYLON.ParticleSystem "resetTestPS"), configure emitter and set options,
then trigger particle emission (e.g., call the public emit/start method or
directly populate ps._particles and ps._stockParticles with mock particle
objects) so that ps._particles and ps._stockParticles are non-empty, call
flock.resetParticleSystem("resetTestPS"), and assert both arrays are emptied;
also add a short comment noting the test relies on internal properties
(_particles/_stockParticles) if no public API exists.
Comment on lines
+38
to
+50
| it("returns false for reserved prefixes", function () { | ||
| expect(flock.isAllowedEventName("onSomething")).to.be.false; | ||
| expect(flock.isAllowedEventName("systemEvent")).to.be.false; | ||
| expect(flock.isAllowedEventName("internalMsg")).to.be.false; | ||
| expect(flock.isAllowedEventName("babylonTick")).to.be.false; | ||
| expect(flock.isAllowedEventName("flockReady")).to.be.false; | ||
| expect(flock.isAllowedEventName("_hidden")).to.be.false; | ||
| }); | ||
|
|
||
| it("returns false for disallowed characters", function () { | ||
| expect(flock.isAllowedEventName("hello!")).to.be.false; | ||
| expect(flock.isAllowedEventName("say@world")).to.be.false; | ||
| }); |
There was a problem hiding this comment.
These expectations don't match isAllowedEventName() today.
The current implementation in api/events.js only rejects non-strings, names longer than 30 characters, and prefixes babylon|pointer|keys|mouse|touch|camera|mic|scene. The onSomething/systemEvent/internalMsg/flockReady/_hidden and punctuation cases here still return true, so this block will fail unless the validator changes too.
Align the test with the current contract
it("returns false for reserved prefixes", function () {
- expect(flock.isAllowedEventName("onSomething")).to.be.false;
- expect(flock.isAllowedEventName("systemEvent")).to.be.false;
- expect(flock.isAllowedEventName("internalMsg")).to.be.false;
expect(flock.isAllowedEventName("babylonTick")).to.be.false;
- expect(flock.isAllowedEventName("flockReady")).to.be.false;
- expect(flock.isAllowedEventName("_hidden")).to.be.false;
+ expect(flock.isAllowedEventName("pointerDown")).to.be.false;
+ expect(flock.isAllowedEventName("sceneLoaded")).to.be.false;
});
-
- it("returns false for disallowed characters", function () {
- expect(flock.isAllowedEventName("hello!")).to.be.false;
- expect(flock.isAllowedEventName("say@world")).to.be.false;
- });📝 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.
Suggested change
| it("returns false for reserved prefixes", function () { | |
| expect(flock.isAllowedEventName("onSomething")).to.be.false; | |
| expect(flock.isAllowedEventName("systemEvent")).to.be.false; | |
| expect(flock.isAllowedEventName("internalMsg")).to.be.false; | |
| expect(flock.isAllowedEventName("babylonTick")).to.be.false; | |
| expect(flock.isAllowedEventName("flockReady")).to.be.false; | |
| expect(flock.isAllowedEventName("_hidden")).to.be.false; | |
| }); | |
| it("returns false for disallowed characters", function () { | |
| expect(flock.isAllowedEventName("hello!")).to.be.false; | |
| expect(flock.isAllowedEventName("say@world")).to.be.false; | |
| }); | |
| it("returns false for reserved prefixes", function () { | |
| expect(flock.isAllowedEventName("babylonTick")).to.be.false; | |
| expect(flock.isAllowedEventName("pointerDown")).to.be.false; | |
| expect(flock.isAllowedEventName("sceneLoaded")).to.be.false; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/events.test.js` around lines 38 - 50, The test expectations mismatch
the current validator contract in isAllowedEventName: update tests in
tests/events.test.js to reflect that isAllowedEventName only rejects
non-strings, names longer than 30 chars, and names starting with the reserved
prefixes babylon|pointer|keys|mouse|touch|camera|mic|scene; change the "returns
false for reserved prefixes" block to assert true for names like
"onSomething","systemEvent","internalMsg","flockReady","_hidden" (or remove that
block), and instead add/keep assertions that names with the actual reserved
prefixes (e.g. "babylonTick","pointerMove") return false and that overly long
strings and non-string inputs return false while punctuation (e.g.
"hello!","say@world") remains allowed.
Comment on lines
+359
to
+371
| let count = 0; | ||
| flock.onTrigger(id1, { | ||
| trigger: "OnPickTrigger", | ||
| callback: () => count++, | ||
| applyToGroup: false, | ||
| }); | ||
|
|
||
| // Trigger the second mesh — should not fire since only id1 was registered | ||
| mesh2.actionManager?.processTrigger( | ||
| flock.BABYLON.ActionManager.OnPickTrigger, | ||
| ); | ||
|
|
||
| expect(count).to.equal(0); |
There was a problem hiding this comment.
Add a positive control for applyToGroup: false.
This only proves mesh2 does not fire. If flock.onTrigger() fails to register anything, the test still passes. Trigger mesh1 once as well so the case verifies both the positive and negative paths.
Possible fix
flock.onTrigger(id1, {
trigger: "OnPickTrigger",
callback: () => count++,
applyToGroup: false,
});
+
+ mesh1.actionManager?.processTrigger(
+ flock.BABYLON.ActionManager.OnPickTrigger,
+ );
+ expect(count).to.equal(1);
// Trigger the second mesh — should not fire since only id1 was registered
mesh2.actionManager?.processTrigger(
flock.BABYLON.ActionManager.OnPickTrigger,
);
- expect(count).to.equal(0);
+ expect(count).to.equal(1);📝 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.
Suggested change
| let count = 0; | |
| flock.onTrigger(id1, { | |
| trigger: "OnPickTrigger", | |
| callback: () => count++, | |
| applyToGroup: false, | |
| }); | |
| // Trigger the second mesh — should not fire since only id1 was registered | |
| mesh2.actionManager?.processTrigger( | |
| flock.BABYLON.ActionManager.OnPickTrigger, | |
| ); | |
| expect(count).to.equal(0); | |
| let count = 0; | |
| flock.onTrigger(id1, { | |
| trigger: "OnPickTrigger", | |
| callback: () => count++, | |
| applyToGroup: false, | |
| }); | |
| mesh1.actionManager?.processTrigger( | |
| flock.BABYLON.ActionManager.OnPickTrigger, | |
| ); | |
| expect(count).to.equal(1); | |
| // Trigger the second mesh — should not fire since only id1 was registered | |
| mesh2.actionManager?.processTrigger( | |
| flock.BABYLON.ActionManager.OnPickTrigger, | |
| ); | |
| expect(count).to.equal(1); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/events.test.js` around lines 359 - 371, Test currently only asserts
mesh2 does not fire; add a positive control to verify the registration worked by
triggering mesh1 and asserting the counter increments. After calling
flock.onTrigger(id1, { trigger: "OnPickTrigger", callback: () => count++,
applyToGroup: false }), call
mesh1.actionManager?.processTrigger(flock.BABYLON.ActionManager.OnPickTrigger)
and assert expect(count).to.equal(1) before running the mesh2 negative assertion
(which should then assert count remains 1 or equals 1 after mesh2 triggers).
Comment on lines
+359
to
+362
| it("mesh metadata has the correct modelName", function () { | ||
| const mesh = flock.scene.getMeshByName(meshId); | ||
| expect(mesh.metadata?.modelName).to.equal(modelName); | ||
| }); |
There was a problem hiding this comment.
Test asserts wrong metadata field — will fail.
The test expects mesh.metadata?.modelName but per api/models.js:358, the implementation sets metadata.templateTag, not metadata.modelName.
🐛 Proposed fix
- it("mesh metadata has the correct modelName", function () {
+ it("mesh metadata has the correct templateTag", function () {
const mesh = flock.scene.getMeshByName(meshId);
- expect(mesh.metadata?.modelName).to.equal(modelName);
+ expect(mesh.metadata?.templateTag).to.equal(modelName);
});📝 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.
Suggested change
| it("mesh metadata has the correct modelName", function () { | |
| const mesh = flock.scene.getMeshByName(meshId); | |
| expect(mesh.metadata?.modelName).to.equal(modelName); | |
| }); | |
| it("mesh metadata has the correct templateTag", function () { | |
| const mesh = flock.scene.getMeshByName(meshId); | |
| expect(mesh.metadata?.templateTag).to.equal(modelName); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/objects.test.js` around lines 359 - 362, The test is asserting the
wrong metadata field; change the assertion in the test (the it block referencing
mesh, meshId, and modelName) to check mesh.metadata?.templateTag instead of
mesh.metadata?.modelName so it matches the implementation in api/models.js which
sets metadata.templateTag; ensure the expectation compares templateTag to
modelName.
Comment on lines
+478
to
+492
| it("should set physicsShapeType to CAPSULE on the mesh metadata", async function () { | ||
| const id = "physicsShapeBox1"; | ||
| flock.createBox(id, { | ||
| width: 1, | ||
| height: 2, | ||
| depth: 1, | ||
| position: [0, 1, 0], | ||
| }); | ||
| boxIds.push(id); | ||
|
|
||
| await flock.setPhysicsShape(id, "CAPSULE"); | ||
|
|
||
| const mesh = flock.scene.getMeshByName(id); | ||
| expect(mesh.metadata.physicsShapeType).to.equal("CAPSULE"); | ||
| }); |
There was a problem hiding this comment.
Missing await on flock.createBox may cause race condition.
flock.createBox is async (as shown elsewhere in this file), but it's called without await here. The subsequent setPhysicsShape call may execute before the box is fully created.
🐛 Proposed fix
it("should set physicsShapeType to CAPSULE on the mesh metadata", async function () {
const id = "physicsShapeBox1";
- flock.createBox(id, {
+ await flock.createBox(id, {
width: 1,
height: 2,
depth: 1,
position: [0, 1, 0],
});
boxIds.push(id);📝 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.
Suggested change
| it("should set physicsShapeType to CAPSULE on the mesh metadata", async function () { | |
| const id = "physicsShapeBox1"; | |
| flock.createBox(id, { | |
| width: 1, | |
| height: 2, | |
| depth: 1, | |
| position: [0, 1, 0], | |
| }); | |
| boxIds.push(id); | |
| await flock.setPhysicsShape(id, "CAPSULE"); | |
| const mesh = flock.scene.getMeshByName(id); | |
| expect(mesh.metadata.physicsShapeType).to.equal("CAPSULE"); | |
| }); | |
| it("should set physicsShapeType to CAPSULE on the mesh metadata", async function () { | |
| const id = "physicsShapeBox1"; | |
| await flock.createBox(id, { | |
| width: 1, | |
| height: 2, | |
| depth: 1, | |
| position: [0, 1, 0], | |
| }); | |
| boxIds.push(id); | |
| await flock.setPhysicsShape(id, "CAPSULE"); | |
| const mesh = flock.scene.getMeshByName(id); | |
| expect(mesh.metadata.physicsShapeType).to.equal("CAPSULE"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/physics.test.js` around lines 478 - 492, The test "should set
physicsShapeType to CAPSULE on the mesh metadata" calls the async factory
function flock.createBox without awaiting it, which can cause a race with the
subsequent flock.setPhysicsShape call; update the test to await
flock.createBox(id, {...}) before pushing id into boxIds and before calling
flock.setPhysicsShape so the mesh exists (refer to flock.createBox and
flock.setPhysicsShape in the test).
Comment on lines
+494
to
+508
| it("should set physicsShapeType to MESH on the mesh metadata", async function () { | ||
| const id = "physicsShapeBox2"; | ||
| flock.createBox(id, { | ||
| width: 1, | ||
| height: 1, | ||
| depth: 1, | ||
| position: [3, 0, 0], | ||
| }); | ||
| boxIds.push(id); | ||
|
|
||
| await flock.setPhysicsShape(id, "MESH"); | ||
|
|
||
| const mesh = flock.scene.getMeshByName(id); | ||
| expect(mesh.metadata.physicsShapeType).to.equal("MESH"); | ||
| }); |
There was a problem hiding this comment.
Missing await on flock.createBox — same issue as above.
🐛 Proposed fix
it("should set physicsShapeType to MESH on the mesh metadata", async function () {
const id = "physicsShapeBox2";
- flock.createBox(id, {
+ await flock.createBox(id, {
width: 1,
height: 1,
depth: 1,
position: [3, 0, 0],
});
boxIds.push(id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/physics.test.js` around lines 494 - 508, The test is missing an await
when calling flock.createBox which can cause a race before setPhysicsShape runs;
update the test to await flock.createBox(id, {...}) and only push id into boxIds
after the awaited creation completes so that subsequent calls
(flock.setPhysicsShape and flock.scene.getMeshByName) operate on the
fully-created mesh (refer to createBox, setPhysicsShape, getMeshByName, and
mesh.metadata.physicsShapeType).
Comment on lines
+280
to
+290
| let savedScene; | ||
|
|
||
| before(function () { | ||
| savedScene = flock.scene; | ||
| flock.engine?.stopRenderLoop(); | ||
| }); | ||
|
|
||
| after(function () { | ||
| flock.scene = savedScene; | ||
| flock.engine?.runRenderLoop(flock._renderLoop); | ||
| }); |
There was a problem hiding this comment.
Restore the full initialize() fixture, not just scene.
initialize() mutates more than flock.scene here: it sets up engine, canvas, observables, and abortController. Restoring only scene leaves the newly initialized globals on the shared fixture and can pollute later suites.
Possible fix
- let savedScene;
+ let savedScene;
+ let savedEngine;
+ let savedCanvas;
+ let savedAbortController;
+ let savedGridKeyPressObservable;
+ let savedGridKeyReleaseObservable;
before(function () {
+ savedEngine = flock.engine;
savedScene = flock.scene;
+ savedCanvas = flock.canvas;
+ savedAbortController = flock.abortController;
+ savedGridKeyPressObservable = flock.gridKeyPressObservable;
+ savedGridKeyReleaseObservable = flock.gridKeyReleaseObservable;
flock.engine?.stopRenderLoop();
});
after(function () {
+ if (flock.engine && flock.engine !== savedEngine) {
+ flock.engine.dispose();
+ }
+ flock.engine = savedEngine;
flock.scene = savedScene;
+ flock.canvas = savedCanvas;
+ flock.abortController = savedAbortController;
+ flock.gridKeyPressObservable = savedGridKeyPressObservable;
+ flock.gridKeyReleaseObservable = savedGridKeyReleaseObservable;
flock.engine?.runRenderLoop(flock._renderLoop);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/scene.test.js` around lines 280 - 290, The test currently only
saves/restores flock.scene but initialize() also mutates flock.engine,
flock.canvas, observables, abortController and the render loop; update the
fixture to snapshot the full pre-initialize state (e.g. save savedEngine,
savedCanvas, savedScene, savedAbortController and any observable references) in
before and restore each in after, ensuring you stop the render loop with
flock.engine?.stopRenderLoop() before the test and restart the original with
flock.engine?.runRenderLoop(flock._renderLoop) after restoring the original
engine and render loop; reference symbols: flock.initialize(), flock.engine,
flock.canvas, flock.scene, flock.abortController, flock._renderLoop and any
observable properties you saved.
Comment on lines
+22
to
+36
| it("should set bpm on mesh metadata for a named mesh", async function () { | ||
| const id = "bpmTestBox"; | ||
| flock.createBox(id, { | ||
| width: 1, | ||
| height: 1, | ||
| depth: 1, | ||
| position: [0, 0, 0], | ||
| }); | ||
|
|
||
| await flock.setBPM(id, 90); | ||
|
|
||
| const mesh = flock.scene.getMeshByName(id); | ||
| expect(mesh.metadata.bpm).to.equal(90); | ||
| flock.dispose(id); | ||
| }); |
There was a problem hiding this comment.
Missing await on flock.createBox.
flock.createBox is async, but it's not awaited here. The subsequent setBPM call may execute before the box is fully created, potentially causing the mesh lookup to fail.
🐛 Proposed fix
it("should set bpm on mesh metadata for a named mesh", async function () {
const id = "bpmTestBox";
- flock.createBox(id, {
+ await flock.createBox(id, {
width: 1,
height: 1,
depth: 1,
position: [0, 0, 0],
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/sound2.test.js` around lines 22 - 36, The test calls the async helper
flock.createBox without awaiting it, so setBPM and the mesh lookup may run
before the box exists; update the test to await flock.createBox(id, { ... })
inside the async it block (i.e., add await before the flock.createBox call in
the test that uses setBPM, then keep the rest: await flock.setBPM(id, 90), const
mesh = flock.scene.getMeshByName(id), expect(mesh.metadata.bpm).to.equal(90),
flock.dispose(id)).
Comment on lines
+52
to
+66
| it("should not throw when called with a mesh name and text", async function () { | ||
| const id = "speakTestBox"; | ||
| flock.createBox(id, { | ||
| width: 1, | ||
| height: 1, | ||
| depth: 1, | ||
| position: [0, 0, 0], | ||
| }); | ||
|
|
||
| try { | ||
| await speakWithTimeout(() => flock.speak(id, "hello")); | ||
| } finally { | ||
| flock.dispose(id); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing await on flock.createBox — same issue.
🐛 Proposed fix
it("should not throw when called with a mesh name and text", async function () {
const id = "speakTestBox";
- flock.createBox(id, {
+ await flock.createBox(id, {
width: 1,
height: 1,
depth: 1,
position: [0, 0, 0],
});📝 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.
Suggested change
| it("should not throw when called with a mesh name and text", async function () { | |
| const id = "speakTestBox"; | |
| flock.createBox(id, { | |
| width: 1, | |
| height: 1, | |
| depth: 1, | |
| position: [0, 0, 0], | |
| }); | |
| try { | |
| await speakWithTimeout(() => flock.speak(id, "hello")); | |
| } finally { | |
| flock.dispose(id); | |
| } | |
| }); | |
| it("should not throw when called with a mesh name and text", async function () { | |
| const id = "speakTestBox"; | |
| await flock.createBox(id, { | |
| width: 1, | |
| height: 1, | |
| depth: 1, | |
| position: [0, 0, 0], | |
| }); | |
| try { | |
| await speakWithTimeout(() => flock.speak(id, "hello")); | |
| } finally { | |
| flock.dispose(id); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/sound2.test.js` around lines 52 - 66, The test calls flock.createBox
without awaiting it, which can cause race conditions before calling
speakWithTimeout and flock.speak; add await before flock.createBox(id, {...}) so
the box is fully created before invoking speakWithTimeout(() => flock.speak(id,
"hello")), and ensure flock.dispose(id) remains in the finally block; apply the
same fix for any other tests using flock.createBox without await.
This was referenced Mar 29, 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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit (editted for correctness by @tracygardner)
New Features
Removals
createGroundfrom the sandbox API whitelist.Improvements