Update duplicate placement to track and switch source mesh and highlight chained duplicates#575
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe change introduces Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Deploying flockxr with
|
| Latest commit: |
2ac4395
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://05d27bad.flockxr.pages.dev |
| Branch Preview URL: | https://codex-investigate-duplicate.flockxr.pages.dev |
![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
🧹 Nitpick comments (1)
ui/gizmos.js (1)
999-1000: Nit:|| newBlock.idfallback is dead.
getMeshFromBlockKeylooks up bymesh.metadata.blockKey, not by Blockly block id, so substitutingnewBlock.idwhengetBlockKeyFromBlockreturns null cannot match any entry inblockKeyToMeshes. The real fallback is|| getMeshFromBlock(newBlock)on the next line. The|| newBlock.idonly avoids passingnullintogetMeshFromBlockKey(which already early-returns on a falsy key), so it is effectively a no-op.- const newBlockKey = getBlockKeyFromBlock(newBlock) || newBlock.id; - let nextSource = getMeshFromBlockKey(newBlockKey) || getMeshFromBlock(newBlock); + const newBlockKey = getBlockKeyFromBlock(newBlock); + let nextSource = + (newBlockKey && getMeshFromBlockKey(newBlockKey)) || + getMeshFromBlock(newBlock);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/gizmos.js` around lines 999 - 1000, The fallback "|| newBlock.id" is dead because getMeshFromBlockKey looks up mesh.metadata.blockKey (not Blockly block ids), so supplying newBlock.id never yields a match; remove the "|| newBlock.id" and instead rely on the existing fallback to getMeshFromBlock(newBlock). Update the code around getBlockKeyFromBlock(newBlock) and getMeshFromBlockKey/getMeshFromBlock so you pass the real blockKey or null and let getMeshFromBlock(newBlock) run as the true fallback; reference getBlockKeyFromBlock, getMeshFromBlockKey, getMeshFromBlock and blockKeyToMeshes when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/gizmos.js`:
- Around line 988-1020: The retry loop in updateDuplicateChainSource uses
requestAnimationFrame without cancelling or guarding, so late callbacks can
mutate meshToClone after duplicate mode exits; modify resolveSourceMesh to store
the RAF handle (e.g., let duplicateRafId) when calling requestAnimationFrame,
guard inside resolveSourceMesh with a check that duplicate mode is still active
(e.g., isDuplicateModeActive() or a duplicateMode flag) before touching
meshToClone, and ensure you cancel any pending RAF via
cancelAnimationFrame(duplicateRafId) from the explicit exit paths (callers like
exitGizmoState(), your click-out/cancel handlers and any toggle handlers) so
pending callbacks are cleared when duplicate mode ends. Ensure the same
meshToClone checks (meshToClone && meshToClone !== nextSource) remain inside the
guarded block.
- Around line 1011-1016: When swapping meshToClone to nextSource, also restore
the previous source's visibility like
resetBoundingBoxVisibilityIfManuallyChanged does: before assigning meshToClone =
nextSource, if meshToClone exists and is not nextSource set
meshToClone.showBoundingBox = false and restore its visibility (e.g. set
meshToClone.visibility back to 1 or its original visible value) so intermediate
sources aren't left at 0.001; update the swap logic around
meshToClone/nextSource and keep references to
gizmoManager.attachedMesh/resetBoundingBoxVisibilityIfManuallyChanged semantics.
---
Nitpick comments:
In `@ui/gizmos.js`:
- Around line 999-1000: The fallback "|| newBlock.id" is dead because
getMeshFromBlockKey looks up mesh.metadata.blockKey (not Blockly block ids), so
supplying newBlock.id never yields a match; remove the "|| newBlock.id" and
instead rely on the existing fallback to getMeshFromBlock(newBlock). Update the
code around getBlockKeyFromBlock(newBlock) and
getMeshFromBlockKey/getMeshFromBlock so you pass the real blockKey or null and
let getMeshFromBlock(newBlock) run as the true fallback; reference
getBlockKeyFromBlock, getMeshFromBlockKey, getMeshFromBlock and blockKeyToMeshes
when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if (meshToClone && meshToClone !== nextSource) { | ||
| meshToClone.showBoundingBox = false; | ||
| } | ||
| meshToClone = nextSource; | ||
| meshToClone.visibility = 0.001; | ||
| meshToClone.showBoundingBox = true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm there is no other place that walks the chain to restore visibility
# on intermediate meshes after a chained duplicate.
rg -nP -C3 '\bvisibility\s*=\s*(0|1|0\.001)\b' --type=js -g '!**/node_modules/**'Repository: flipcomputing/flock
Length of output: 50376
🏁 Script executed:
# Find all call sites of resetBoundingBoxVisibilityIfManuallyChanged
rg -n 'resetBoundingBoxVisibilityIfManuallyChanged' ui/gizmos.jsRepository: flipcomputing/flock
Length of output: 207
🏁 Script executed:
# Check context around line 1736
sed -n '1720,1750p' ui/gizmos.jsRepository: flipcomputing/flock
Length of output: 1024
🏁 Script executed:
# Check context around lines 1011-1016 (the chain switch logic)
sed -n '990,1025p' ui/gizmos.jsRepository: flipcomputing/flock
Length of output: 1090
🏁 Script executed:
# Search for where duplicate mode is activated/deactivated and any cleanup
rg -n -B2 -A2 'activeDuplicatePickHandler|toggleDuplicate|toggleGizmo' ui/gizmos.js | head -100Repository: flipcomputing/flock
Length of output: 1977
🏁 Script executed:
# Find exitGizmoState function and see what cleanup it does
rg -n -A15 'function exitGizmoState|const exitGizmoState' ui/gizmos.jsRepository: flipcomputing/flock
Length of output: 524
🏁 Script executed:
# Find runCleanups function and what cleanup functions are queued
rg -n -B5 -A10 'function runCleanups|const runCleanups|cleanups\.push' ui/gizmos.js | head -80Repository: flipcomputing/flock
Length of output: 532
🏁 Script executed:
# Find where onExit is called during duplicate mode setup (search around line 1068)
sed -n '1050,1120p' ui/gizmos.jsRepository: flipcomputing/flock
Length of output: 2117
🏁 Script executed:
# Search for where onExit is called during duplicate mode (if at all)
rg -n 'onExit\(' ui/gizmos.jsRepository: flipcomputing/flock
Length of output: 466
🏁 Script executed:
# Find updateDuplicateChainSource which is called after each placement
rg -n -A20 'function updateDuplicateChainSource' ui/gizmos.jsRepository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
# Search for updateDuplicateChainSource differently
rg -n 'updateDuplicateChainSource' ui/gizmos.jsRepository: flipcomputing/flock
Length of output: 255
🏁 Script executed:
# Show the updateDuplicateChainSource function
sed -n '988,1010p' ui/gizmos.jsRepository: flipcomputing/flock
Length of output: 756
🏁 Script executed:
# Search for all references to meshToClone to understand the full flow
rg -n 'meshToClone' ui/gizmos.jsRepository: flipcomputing/flock
Length of output: 583
🏁 Script executed:
# Check what happens at lines 1027, 1052, 1085 where showBoundingBox is set to false
sed -n '1025,1030p' ui/gizmos.js
sed -n '1050,1055p' ui/gizmos.js
sed -n '1083,1088p' ui/gizmos.jsRepository: flipcomputing/flock
Length of output: 680
Reset visibility of previous source when swapping meshToClone in the duplicate chain.
Each successful duplicate placement sets nextSource.visibility = 0.001 (line 1015). When the source switches to the next placement, only showBoundingBox is cleared on the previous source (line 1012); its visibility remains at 0.001. After a chain of N placements, every intermediate source mesh is left invisible at 0.001 visibility. resetBoundingBoxVisibilityIfManuallyChanged only restores the final gizmoManager.attachedMesh, leaving intermediates unreset. For non-composite primitives where the root is the visible geometry, this makes intermediate duplicates effectively invisible.
Restore visibility when swapping meshToClone to match the logic in resetBoundingBoxVisibilityIfManuallyChanged:
Suggested change
if (meshToClone && meshToClone !== nextSource) {
meshToClone.showBoundingBox = false;
+ // matches resetBoundingBoxVisibilityIfManuallyChanged
+ if (meshToClone.visibility === 0.001) meshToClone.visibility = 0;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/gizmos.js` around lines 1011 - 1016, When swapping meshToClone to
nextSource, also restore the previous source's visibility like
resetBoundingBoxVisibilityIfManuallyChanged does: before assigning meshToClone =
nextSource, if meshToClone exists and is not nextSource set
meshToClone.showBoundingBox = false and restore its visibility (e.g. set
meshToClone.visibility back to 1 or its original visible value) so intermediate
sources aren't left at 0.001; update the swap logic around
meshToClone/nextSource and keep references to
gizmoManager.attachedMesh/resetBoundingBoxVisibilityIfManuallyChanged semantics.
tracygardner
deleted the
codex/investigate-duplicate-tool-selection-issue
branch
1 participant
Motivation
Description
meshToClonefromconsttoletso it can be updated when the duplicate chain source changes.updateDuplicateChainSourcewhich highlights the new block, updatesblockId, sets the crosshair cursor, resolves the new source mesh (with an animation-frame retry loop), and updatesmeshToClonevisibility and bounding box.highlightBlockByIdcalls afterduplicateBlockAndInsertwithupdateDuplicateChainSource(newBlock, workspace)in both click and keyboard placement paths.Testing
npm test, which completed successfully.npm run lint, which passed without errors.Codex Task
Summary by CodeRabbit