Use flock._reserveName for name allocation across creators and update tests#507
Merged
tracygardner merged 2 commits intomainfrom Apr 5, 2026
Merged
Conversation
Deploying flockdev with
|
| Latest commit: |
dd902f9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ad91c6c7.flockdev.pages.dev |
| Branch Preview URL: | https://codex-refactor-name-allocati.flockdev.pages.dev |
📝 WalkthroughWalkthroughConsolidates name/ID allocation by replacing ad-hoc collision checks and unique-suffixing with unconditional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/shapes.test.js (1)
1-1:⚠️ Potential issue | 🟡 MinorFix Prettier formatting issue.
The CI pipeline reports a Prettier formatting violation in this file. Run
npx prettier --write tests/shapes.test.jsto resolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/shapes.test.js` at line 1, Prettier flagged formatting in tests/shapes.test.js; run the formatter (e.g., npx prettier --write tests/shapes.test.js) or apply Prettier rules to reformat the file so the import statement (import { expect } from "chai";) and the rest of tests/shapes.test.js conform to the project's Prettier configuration, then re-stage the reformatted file and push the commit.
🧹 Nitpick comments (1)
api/scene.js (1)
642-689: Consider releasing reserved name on failure paths.If
sourceMeshis null/disposed (line 643 returns early), or ifcloneis falsy (line 651), the reserved name remains in_nameRegistrybut is never used. Per the_releaseNamehelper inflock.js, reservations should be released on failure.This is an optional improvement that could be addressed in a follow-up.
💡 Suggested enhancement for failure paths
flock.whenModelReady(sourceMeshName, (sourceMesh) => { - if (!sourceMesh || sourceMesh.isDisposed?.()) return; + if (!sourceMesh || sourceMesh.isDisposed?.()) { + flock._releaseName(uniqueCloneId); + return; + } flock._recycleOldestByKey(sourceMeshName); const clone = sourceMesh.clone(uniqueCloneId); + if (!clone) { + flock._releaseName(uniqueCloneId); + return; + } + flock._registerInstance(sourceMeshName, clone.name); - if (clone) { sourceMesh.computeWorldMatrix(true); // ... rest of the code - } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/scene.js` around lines 642 - 689, The reserved clone name registered via flock._registerInstance(uniqueCloneId) can be leaked when sourceMesh is null/disposed or when clone is falsy; update the flock.whenModelReady callback to call the name-release helper (flock._releaseName or the corresponding release API) before any early return: call it immediately when sourceMesh is null/disposed and also when clone is falsy after registering (then return), ensuring the reservation is freed in both failure paths.
🤖 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/effects.test.js`:
- Around line 82-132: The test wraps flock._reserveName but calls createBox
while the wrapper is active which may append extra names to reserveCalls; move
the call to flock.createBox("effectReserveEmitter", ...) to before you replace
flock._reserveName (i.e., create the emitterMesh first), then install the
wrapper and call flock.createParticleEffect twice to populate reserveCalls;
additionally ensure you restore flock._reserveName (using originalReserveName)
before invoking done/done(error) by restoring inside the try/catch (or a finally
that restores and then calls done) so the original _reserveName is reinstated
prior to ending the test; refer to flock._reserveName, originalReserveName,
reserveCalls, createBox, createParticleEffect, createdMeshes and createdEffects
when making these edits.
In `@tests/scene.test.js`:
- Around line 394-428: The test wraps flock._reserveName to record calls but
installs that wrapper before calling createBox, and createBox itself calls
flock._reserveName, so reserveCalls includes the box name; fix by creating the
box first (call flock.createBox(...) and push boxId into createdIds), then
install the wrapper around flock._reserveName to capture reserveClone calls,
then perform the two flock.cloneMesh(...) calls; keep restoring
flock._reserveName in the finally block and leave assertions
(expect(reserveCalls)... and expect(firstCloneId)... ) unchanged.
---
Outside diff comments:
In `@tests/shapes.test.js`:
- Line 1: Prettier flagged formatting in tests/shapes.test.js; run the formatter
(e.g., npx prettier --write tests/shapes.test.js) or apply Prettier rules to
reformat the file so the import statement (import { expect } from "chai";) and
the rest of tests/shapes.test.js conform to the project's Prettier
configuration, then re-stage the reformatted file and push the commit.
---
Nitpick comments:
In `@api/scene.js`:
- Around line 642-689: The reserved clone name registered via
flock._registerInstance(uniqueCloneId) can be leaked when sourceMesh is
null/disposed or when clone is falsy; update the flock.whenModelReady callback
to call the name-release helper (flock._releaseName or the corresponding release
API) before any early return: call it immediately when sourceMesh is
null/disposed and also when clone is falsy after registering (then return),
ensuring the reservation is freed in both failure paths.
🪄 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: 18956415-872e-4749-9a6d-764a9346128b
📒 Files selected for processing (10)
api/csg.jsapi/effects.jsapi/scene.jsapi/shapes.jstests/characterAnimations.test.jstests/effects.test.jstests/materials.test.jstests/objects.test.jstests/scene.test.jstests/shapes.test.js
tests/effects.test.js
Outdated
Comment on lines
+82
to
+132
| it("should route particle effect name allocation through _reserveName", function (done) { | ||
| this.timeout(5000); | ||
| const originalReserveName = flock._reserveName; | ||
| const reserveCalls = []; | ||
|
|
||
| flock._reserveName = (name) => { | ||
| reserveCalls.push(name); | ||
| return originalReserveName.call(flock, name); | ||
| }; | ||
|
|
||
| const emitterId = flock.createBox("effectReserveEmitter", { | ||
| color: "#FF0000", | ||
| width: 1, | ||
| height: 1, | ||
| depth: 1, | ||
| position: [0, 0, 0], | ||
| }); | ||
| createdMeshes.push(emitterId); | ||
|
|
||
| const firstEffectName = flock.createParticleEffect("reserveEffect", { | ||
| emitterMesh: emitterId, | ||
| emitRate: 10, | ||
| colors: { start: "#ffffff", end: "#ffffff" }, | ||
| alphas: { start: 1, end: 0 }, | ||
| sizes: { start: 1, end: 1 }, | ||
| lifetime: { min: 0.1, max: 0.2 }, | ||
| shape: "flare.png", | ||
| }); | ||
| const secondEffectName = flock.createParticleEffect("reserveEffect", { | ||
| emitterMesh: emitterId, | ||
| emitRate: 10, | ||
| colors: { start: "#ffffff", end: "#ffffff" }, | ||
| alphas: { start: 1, end: 0 }, | ||
| sizes: { start: 1, end: 1 }, | ||
| lifetime: { min: 0.1, max: 0.2 }, | ||
| shape: "flare.png", | ||
| }); | ||
| createdEffects.push(firstEffectName, secondEffectName); | ||
|
|
||
| setTimeout(() => { | ||
| try { | ||
| expect(reserveCalls).to.deep.equal(["reserveEffect", "reserveEffect"]); | ||
| expect(firstEffectName).to.not.equal(secondEffectName); | ||
| done(); | ||
| } catch (error) { | ||
| done(error); | ||
| } finally { | ||
| flock._reserveName = originalReserveName; | ||
| } | ||
| }, 100); | ||
| }); |
There was a problem hiding this comment.
Two issues with this test: assertion fragility and finally placement.
-
Same fragility issue:
createBoxon line 92 is called while the wrapper is active. IfcreateBoxalso routes through_reserveName(per the PR summary), the assertion on line 123 will fail. -
finallyruns afterdone(): The current structure callsdone()ordone(error)before thefinallyblock restores_reserveName. While this works, if any subsequent test runs before the finally block completes (unlikely but possible), it could see the wrapped version.
🔧 Proposed fix
it("should route particle effect name allocation through _reserveName", function (done) {
this.timeout(5000);
const originalReserveName = flock._reserveName;
const reserveCalls = [];
flock._reserveName = (name) => {
reserveCalls.push(name);
return originalReserveName.call(flock, name);
};
+ try {
const emitterId = flock.createBox("effectReserveEmitter", {
color: "#FF0000",
width: 1,
height: 1,
depth: 1,
position: [0, 0, 0],
});
createdMeshes.push(emitterId);
+ } finally {
+ // Restore before effect creation to isolate the test
+ }
+
+ // Re-wrap for effect creation only
+ flock._reserveName = (name) => {
+ reserveCalls.push(name);
+ return originalReserveName.call(flock, name);
+ };
const firstEffectName = flock.createParticleEffect("reserveEffect", {
// ...
});
// ...
setTimeout(() => {
try {
- expect(reserveCalls).to.deep.equal(["reserveEffect", "reserveEffect"]);
+ expect(reserveCalls.filter(n => n === "reserveEffect")).to.have.lengthOf(2);
expect(firstEffectName).to.not.equal(secondEffectName);
+ flock._reserveName = originalReserveName;
done();
} catch (error) {
+ flock._reserveName = originalReserveName;
done(error);
- } finally {
- flock._reserveName = originalReserveName;
}
}, 100);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/effects.test.js` around lines 82 - 132, The test wraps
flock._reserveName but calls createBox while the wrapper is active which may
append extra names to reserveCalls; move the call to
flock.createBox("effectReserveEmitter", ...) to before you replace
flock._reserveName (i.e., create the emitterMesh first), then install the
wrapper and call flock.createParticleEffect twice to populate reserveCalls;
additionally ensure you restore flock._reserveName (using originalReserveName)
before invoking done/done(error) by restoring inside the try/catch (or a finally
that restores and then calls done) so the original _reserveName is reinstated
prior to ending the test; refer to flock._reserveName, originalReserveName,
reserveCalls, createBox, createParticleEffect, createdMeshes and createdEffects
when making these edits.
tests/scene.test.js
Outdated
Comment on lines
+394
to
+428
| it("should route clone name allocation through _reserveName", function () { | ||
| const originalReserveName = flock._reserveName; | ||
| const reserveCalls = []; | ||
|
|
||
| flock._reserveName = (name) => { | ||
| reserveCalls.push(name); | ||
| return originalReserveName.call(flock, name); | ||
| }; | ||
|
|
||
| try { | ||
| const boxId = flock.createBox("cloneReserveSrc", { | ||
| color: "#996633", | ||
| width: 1, | ||
| height: 1, | ||
| depth: 1, | ||
| position: [0, 0, 0], | ||
| }); | ||
| createdIds.push(boxId); | ||
|
|
||
| const firstCloneId = flock.cloneMesh({ | ||
| sourceMeshName: boxId, | ||
| cloneId: "reserveClone", | ||
| }); | ||
| const secondCloneId = flock.cloneMesh({ | ||
| sourceMeshName: boxId, | ||
| cloneId: "reserveClone", | ||
| }); | ||
| createdIds.push(firstCloneId, secondCloneId); | ||
|
|
||
| expect(reserveCalls).to.deep.equal(["reserveClone", "reserveClone"]); | ||
| expect(firstCloneId).to.not.equal(secondCloneId); | ||
| } finally { | ||
| flock._reserveName = originalReserveName; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if createBox in api/shapes.js calls _reserveName
# Expected: If createBox calls _reserveName, this test's assertion would be incorrect
rg -n '_reserveName' api/shapes.js | head -20Repository: flipcomputing/flock
Length of output: 350
Test assertion will fail—createBox also calls _reserveName.
The shell verification confirms that createBox in api/shapes.js (line 276) calls flock._reserveName. Since the wrapper is installed before createBox("cloneReserveSrc", ...) is called, the reserveCalls array will contain ["cloneReserveSrc", "reserveClone", "reserveClone"], not ["reserveClone", "reserveClone"] as asserted on line 423, causing the test to fail.
Move box creation before the wrapper installation:
Fix: Install wrapper after box creation
+ const boxId = flock.createBox("cloneReserveSrc", {
+ color: "#996633",
+ width: 1,
+ height: 1,
+ depth: 1,
+ position: [0, 0, 0],
+ });
+ createdIds.push(boxId);
+
const originalReserveName = flock._reserveName;
const reserveCalls = [];
flock._reserveName = (name) => {
reserveCalls.push(name);
return originalReserveName.call(flock, name);
};
try {
- const boxId = flock.createBox("cloneReserveSrc", {
- color: "#996633",
- width: 1,
- height: 1,
- depth: 1,
- position: [0, 0, 0],
- });
- createdIds.push(boxId);
-
const firstCloneId = flock.cloneMesh({
sourceMeshName: boxId,
cloneId: "reserveClone",
});🤖 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 - 428, The test wraps
flock._reserveName to record calls but installs that wrapper before calling
createBox, and createBox itself calls flock._reserveName, so reserveCalls
includes the box name; fix by creating the box first (call flock.createBox(...)
and push boxId into createdIds), then install the wrapper around
flock._reserveName to capture reserveClone calls, then perform the two
flock.cloneMesh(...) calls; keep restoring flock._reserveName in the finally
block and leave assertions (expect(reserveCalls)... and expect(firstCloneId)...
) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 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 `@tests/materials.test.js`:
- Line 946: Run Prettier over the failing test file to fix formatting: execute
prettier --write on tests/materials.test.js, stage and commit the updated file,
and push the change so the CI Prettier check passes; ensure you use the
repository’s Prettier config (prettier.config.js/.prettierrc) so the formatting
matches project rules.
🪄 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: 780d00d4-6cfe-4004-bedd-5dec19f2c82d
📒 Files selected for processing (6)
tests/characterAnimations.test.jstests/effects.test.jstests/materials.test.jstests/objects.test.jstests/scene.test.jstests/shapes.test.js
✅ Files skipped from review due to trivial changes (4)
- tests/objects.test.js
- tests/scene.test.js
- tests/effects.test.js
- tests/characterAnimations.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/shapes.test.js
| const mergedMesh = flock.scene.getMeshByName(id); | ||
| expect(mergedMesh.getTotalVertices()).to.be.greaterThan(0); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Address the Prettier formatting warning.
The pipeline reports a Prettier formatting check warning. Please run prettier --write on this file to resolve the formatting issue before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/materials.test.js` at line 946, Run Prettier over the failing test file
to fix formatting: execute prettier --write on tests/materials.test.js, stage
and commit the updated file, and push the change so the CI Prettier check
passes; ensure you use the repository’s Prettier config
(prettier.config.js/.prettierrc) so the formatting matches project rules.
tracygardner
deleted the
codex/refactor-name-allocation-to-use-_reservename
branch
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.
Motivation
Description
scene.getMeshByNamechecks and manual_ + getUniqueId()suffixing with calls toflock._reserveName(...)in multiple creators includingcreateBox,createSphere,createCylinder,createCapsule,createPlane,create3DText,cloneMesh,createParticleEffect, and CSG identity resolution (resolveCsgModelIdentity).flock._reserveName.flock._reserveName.Testing
tests/shapes.test.js,tests/scene.test.js,tests/effects.test.js,tests/materials.test.js,tests/objects.test.js, andtests/characterAnimations.test.jswith the new assertions forflock._reserveNameusage.Codex Task
Summary by CodeRabbit
Refactor
Tests