Skip to content

Refactor mesh tracking to use pending mesh IDs instead of build tokens#504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tracygardner merged 1 commit into from
Apr 1, 2026
Merged

Refactor mesh tracking to use pending mesh IDs instead of build tokens #504

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions api/shapes.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -724,16 +724,14 @@ export const flockShapes = {
[meshId, blockKey] = modelId.split("__");
}

if (flock.scene.getMeshByName(meshId)) {
if (!flock._pendingMeshIds) flock._pendingMeshIds = new Set();
if (flock.scene.getMeshByName(meshId) || flock._pendingMeshIds.has(meshId)) {
meshId = meshId + "_" + flock.scene.getUniqueId();
}
flock._pendingMeshIds.add(meshId);

flock._recycleOldestByKey(blockKey);

if (!flock._pendingTextBuilds) flock._pendingTextBuilds = new Map();
const buildToken = Symbol();
flock._pendingTextBuilds.set(blockKey, buildToken);

const loadPromise = (async () => {
try {
let mesh;
Comment on lines 735 to 737

@coderabbitaicoderabbitaiBotApr 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, locate the file and check its size
find . -type f -name "shapes.js"| head -5

Repository: flipcomputing/flock

Length of output: 97

🏁 Script executed:

# Check the file size to understand what we're working with
wc -l ./api/shapes.js

Repository: flipcomputing/flock

Length of output: 82

🏁 Script executed:

# Read the relevant sections of the file around the mentioned lines
sed -n '730,875p' ./api/shapes.js

Repository: flipcomputing/flock

Length of output: 4898

🏁 Script executed:

# Check how _registerInstance and _recycleOldestByKey are related
rg "_registerInstance|_recycleOldestByKey" ./api/shapes.js -B 2 -A 2

Repository: flipcomputing/flock

Length of output: 1702

🏁 Script executed:

# Search for the definitions of _registerInstance and _recycleOldestByKey
rg "^\s*(this\._registerInstance|_registerInstance\s*=|this\._recycleOldestByKey|_recycleOldestByKey\s*=)" -A 10

Repository: flipcomputing/flock

Length of output: 45

🏁 Script executed:

# Search for the definitions more broadly
rg "_registerInstance\s*=" -A 10

Repository: flipcomputing/flock

Length of output: 45

🏁 Script executed:

# Search for function/method definitions related to _registerInstance and _recycleOldestByKey
rg "_registerInstance|_recycleOldestByKey" --type js -A 5 | head -100

Repository: flipcomputing/flock

Length of output: 4349

🏁 Script executed:

# Look at the _registerInstance and _recycleOldestByKey implementation in flock.js
rg -A 15 "_registerInstance\(key, meshName\)" ./flock.js

Repository: flipcomputing/flock

Length of output: 626

🏁 Script executed:

# Look at the _recycleOldestByKey implementation
rg -A 20 "_recycleOldestByKey\(key\)" ./flock.js

Repository: flipcomputing/flock

Length of output: 997

🏁 Script executed:

# Check how dispose() is used elsewhere in the codebase for meshes
rg "\.dispose\(\)" ./api ./blocks -B 2 -A 2 | head -60

Repository: flipcomputing/flock

Length of output: 2250

🏁 Script executed:

# Check for disposeMesh helper function
rg "disposeMesh" ./flock.js ./api -B 2 -A 5

Repository: flipcomputing/flock

Length of output: 1776

🏁 Script executed:

# Get the full disposeMesh function
rg -A 20 "^\s*disposeMesh\(mesh\)" ./api/scene.js

Repository: flipcomputing/flock

Length of output: 553

🏁 Script executed:

# Check for existing tests related to mesh creation failures or cleanup
find . -type f -name "*.test.js" -o -name "*.spec.js" -o -name "*.test.ts"| head -20

Repository: flipcomputing/flock

Length of output: 575

🏁 Script executed:

# Look for tests related to text/shapes
find ./tests -type f -name "*.test.js"| xargs grep -l "create3DText\|3DText\|text.*shape"2>/dev/null

Repository: flipcomputing/flock

Length of output: 85

🏁 Script executed:

# Check the shapes test file for relevant tests
cat -n ./tests/shapes.test.js | head -100

Repository: flipcomputing/flock

Length of output: 3656

🏁 Script executed:

# Continue reading the shapes test file to see if there are error handling tests
cat -n ./tests/shapes.test.js | tail -n +100

Repository: flipcomputing/flock

Length of output: 260

Clean up partially created meshes on failure.

Meshes created at lines 755 or 806 are added directly to the scene, but _registerInstance isn't called until line 864. If any failure occurs between mesh creation and registration, the mesh remains in the scene but is never added to flock._modelInstances[key]. This breaks mesh recycling—_recycleOldestByKey() filters from _modelInstances[key] and won't find untracked meshes, leaving behind orphaned instances. Subsequent retries collide on the mesh name.

Move the let mesh; declaration outside the try block and add mesh?.dispose(); in the catch block to clean up on failure.

Suggested fix
- const loadPromise = (async () => {- try {- let mesh;+ const loadPromise = (async () => {+ let mesh;+ try {
let fontReferenceHeight = null;
@@
} catch (error) {
+ mesh?.dispose();
flock._pendingMeshIds?.delete(meshId);
console.error(`Error creating 3D text '${meshId}':`, error);
throw error;
}
})();

Also add a regression test that forces a throw after mesh allocation and asserts the failed meshId is removed from the scene before retrying.

📝 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
constloadPromise=(async()=>{
try{
letmesh;
constloadPromise=(async()=>{
letmesh;
try{
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/shapes.js` around lines 735 - 737, The try/catch in loadPromise currently
declares `let mesh` inside the try so any mesh created at the allocation sites
(the lines that add to the scene before `_registerInstance` is called) can be
left orphaned on failure; move the `let mesh` declaration outside the try block
and in the catch block call `mesh?.dispose()` to ensure partially created meshes
are removed from the scene, then rethrow; also add a regression test that
simulates/forces a throw after mesh allocation and asserts the failed `meshId`
is not present in the scene before retrying to prevent collisions with
`_recycleOldestByKey()` and ensure `_registerInstance` invariant with
`flock._modelInstances[key]`.

Expand DownExpand Up@@ -861,18 +859,15 @@ export const flockShapes = {
const textShape = new flock.BABYLON.PhysicsShapeMesh(mesh, flock.scene);
flock.applyPhysics(mesh, textShape);

if (flock._pendingTextBuilds.get(blockKey) !== buildToken) {
mesh.dispose();
return;
}
flock._pendingTextBuilds.delete(blockKey);
flock._pendingMeshIds.delete(meshId);

flock._registerInstance(blockKey, mesh.name);

if (callback) {
requestAnimationFrame(callback);
}
} catch (error) {
flock._pendingMeshIds?.delete(meshId);
console.error(`Error creating 3D text '${meshId}':`, error);
throw error;
}
Expand Down
Loading