Conversation
…e-unity-wearable-preview
…e-unity-wearable-preview
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…e-unity-wearable-preview
…land/builder into feat/spring-bones
| dfsOrder.set(bone.name, order++) | ||
| bone.children.forEach(recursiveVisit) | ||
| } | ||
| bones.filter(bone => !childIds.has(bone.nodeId)).forEach(bone => recursiveVisit(bone.nodeId)) |
There was a problem hiding this comment.
wdyt about doing this in a single for loop?
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: feat: spring bones
Well-structured PR that adds a complete spring bone editing workflow — GLB parsing, Redux state management, live preview integration, and a GLB patching save flow. The architecture is clean and test coverage for the library layer (glbUtils, parseSpringBones, patchGltfSpringBones) is solid. CI is green.
However, I found a correctness issue in the reducer and a critical testing gap that should be addressed before merge.
Git Conventions (ADR-6)
✅ PR title feat: spring bones follows semantic commit format.
✅ Branch name feat/spring-bones follows the <type>/<summary> convention.
Findings
P1 — Major
1. SAVE_ITEM_SUCCESS reducer resets dirty state unconditionally
src/modules/editor/reducer.ts — case SAVE_ITEM_SUCCESS
The reducer updates originalSpringBoneParams to match current springBoneParams for any saved item, not just the one whose spring bones are being edited. If a different item triggers saveItemSuccess (e.g., concurrent save or batch operation), the dirty flag (hasSpringBoneChanges) is silently cleared, and the user loses track of unsaved spring bone edits.
Suggested fix:
case SAVE_ITEM_SUCCESS: {
const savedItemId = action.payload.item?.id
if (savedItemId && savedItemId === state.selectedItemId) {
return { ...state, originalSpringBoneParams: { ...state.springBoneParams } }
}
return state
}2. Zero test coverage for the spring-bone GLB patching save path
src/modules/item/sagas.spec.ts
All existing save-item saga tests stub hasSpringBoneChanges as false. The entire fetch → patch → re-hash → upload sequence — the highest-risk new code path — has no saga-level test coverage. At minimum, add one happy-path test exercising the springBoneHasChanges === true branch.
P2 — Minor
3. Race condition in loadSpringBonesData
src/components/ItemProvider/ItemProvider.tsx
This is an async method called from componentDidUpdate with no cancellation guard. If the user switches items or body shapes quickly, a slow first fetch can overwrite the second result's onSetBones. Consider adding a staleness check after the await calls (e.g., verify this.props.item?.id === item.id).
4. SET_SPRING_BONE_PARAM reducer missing guard for nonexistent bone
src/modules/editor/reducer.ts
If dispatched for a boneName not yet in springBoneParams, spreading undefined creates a partial SpringBoneParams object (only the single field). The UI flow prevents this, but a defensive guard (if (!state.springBoneParams[boneName]) return state) would make the invariant explicit.
5. Shallow copies share gravityDir array references
src/modules/editor/reducer.ts — SET_BONES, RESET_SPRING_BONE_PARAMS, SAVE_ITEM_SUCCESS
{ ...springBoneParams } is a shallow copy. The inner SpringBoneParams objects (including gravityDir tuples) are shared by reference between springBoneParams and originalSpringBoneParams. Currently safe because all mutations create new objects via spread, but fragile if anyone later mutates in-place. Consider deep-copying the inner params:
originalSpringBoneParams[name] = { ...p, gravityDir: [...p.gravityDir] }6. Double JSON serialization in patchGltfSpringBones
src/lib/patchGltfSpringBones.ts
The function serializes json to newJsonBytes, but in the GLB path, buildGlb(chunks.json, ...) serializes it again internally. The first serialization is wasted. Consider removing the newJsonString/newJsonBytes variables from the GLB branch, or reusing them in buildGlb.
7. Non-memoized filter selectors
src/modules/editor/selectors.ts — getSpringBones, getAvatarBones
These use .filter() inline, creating new array references on every call. If any component uses them via useSelector, it will re-render on every unrelated state change. Consider wrapping with createSelector.
8. handlePasteParams dispatches one action per field
src/components/ItemEditorPage/RightPanel/SpringBonesSection/SpringBonesSection.tsx
Pasting parameters dispatches ~5 individual SET_SPRING_BONE_PARAM actions, causing 5 state updates and re-renders in rapid succession. Consider adding a batch action (e.g., setAllSpringBoneParams(boneName, params)) to handle this as a single state update.
Security Review
No critical security issues found. A few hardening suggestions:
- Bone names from GLB are used as object keys: Consider adding a runtime whitelist for the
fieldparameter inSET_SPRING_BONE_PARAMto prevent prototype pollution. - Unbounded JSON chunk parsing: Consider adding a size limit on the JSON chunk in
extractGlbChunksand a node count limit inparseSpringBonesto prevent client-side DoS from crafted GLB files. - Recursive
BoneTreeNoderendering: Add a max depth guard or cycle detection to prevent stack overflow from circular bone hierarchies in malicious files.
Overall, XSS is properly handled (React JSX escaping), content is re-hashed on save (hashV1), and binary operations don't mutate input buffers.
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz via Slack
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Spring Bones Editor — Code Review
PR: #3373 — feat: spring bones
Branch: feat/spring-bones → master
Files changed: 36 (+2740 −28)
CI: ✅ All checks passing
ADR-6: ✅ Branch name feat/spring-bones and PR title feat: spring bones conform to conventions.
Overall Assessment
This is a well-structured, large feature PR. The GLB binary parsing/patching layer is cleanly designed with proper bounds checking, immutability, and separation of concerns. The Redux state management follows established project conventions. Test coverage is solid for the lib layer, actions, reducer, and selectors. I'm approving with several suggestions worth considering.
Findings Summary
- P0 (Blocker): 0
- P1 (Major): 0
- P2 (Minor / Suggestions): 9
P2 Findings
1. [Type Safety] setSpringBoneParam lacks correlated generics — actions.ts
SpringBoneParams[typeof field] resolves to the full union type, so the compiler accepts setSpringBoneParam('bone', 'stiffness', [0,0,0]). A generic signature <K extends keyof SpringBoneParams>(boneName: string, field: K, value: SpringBoneParams[K]) would provide proper correlation.
2. [Data Integrity] Shared mutable gravityDir array reference — parseSpringBones.ts
When gravityDir falls through to the default, parseParams returns a direct reference to DEFAULT_SPRING_BONE_PARAMS.gravityDir. Any downstream mutation would corrupt the shared default. Same issue in the ADD_SPRING_BONE_PARAMS reducer case. Fix: clone with [...DEFAULT_SPRING_BONE_PARAMS.gravityDir].
3. [Robustness] No isFinite() guard on parsed numeric parameters — parseSpringBones.ts
NaN and Infinity pass the typeof === 'number' check. formatNumber(NaN) produces NaN that propagates through the system. Add isFinite() checks alongside the type checks.
4. [Race Condition] loadSpringBonesData in ItemProvider has no cancellation — ItemProvider.tsx
Fire-and-forget async with void. If the user switches items quickly, a slow fetch for item A can resolve after item B's bones are loaded, overwriting fresh state with stale data. Consider adding an item ID guard after await or migrating to a saga-based approach.
5. [Logic] SAVE_ITEM_SUCCESS unconditionally snapshots originalSpringBoneParams — reducer.ts
This action fires for any item save (name change, thumbnail, etc.). It will reset the dirty flag even when the user hasn't saved spring bone changes. Guard with if (action.payload.item.id === state.selectedItemId).
6. [Architecture] Cross-module coupling in save saga — item/sagas.ts
The item save saga directly reads editor state (hasSpringBoneChanges, getBones, getSpringBoneParams, getBodyShape). Consider passing spring bone data through the save action payload instead to keep the module boundary clean.
7. [Performance] getSpringBones and getAvatarBones create new arrays on every call — selectors.ts
These use .filter() which defeats React-Redux shallow equality. Wrap with createSelector for memoization, consistent with hasSpringBoneChanges in the same file.
8. [Code Organization] SpringBonesSection.tsx is 470+ lines with 7 internal components
Consider extracting BoneHierarchyPicker, SpringBoneCard, and the input primitives (InputNumber, SliderInput, Vec3Input) into separate files within the same directory for testability and navigation.
9. [Duplication] bones.filter(b => b.type === 'spring') appears in 3 locations
In selectors.ts, SpringBonesSection.tsx, and patchGltfSpringBones.ts. Consider a shared utility or passing pre-filtered data to reduce duplication.
Security Review
✅ No security issues found.
- Buffer bounds are properly checked in
parseGlb - Input types are validated in
parseParams patchGltfSpringBonesreturns a newArrayBuffer(no input mutation)- Hash re-computation after patching is correctly sequenced
- No secrets, no injection vectors, no XSS risk (React auto-escapes)
Minor hardening suggestions: add isFinite() guards (finding #3 above) and consider a max JSON chunk size limit in parseGlb to prevent client-side DoS from crafted GLB files.
Verdict: ✅ APPROVE
No blockers. The feature is well-implemented with good test coverage, clean binary parsing, and proper Redux patterns. The P2 suggestions above are improvements worth considering but none block merge.
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz via Slack
Add Spring Bones Editor for Wearables
Related PRs:
Context and Problem Statement
Wearable creators need the ability to configure spring bone physics parameters (stiffness, gravity, drag force, etc.) directly within the Builder's item editor. Currently, there is no UI to parse, view, or edit spring bone data embedded in GLB/glTF wearable files, requiring creators to use raw blender custom properties.
Solution
Implement a full spring bones editing workflow: parsing spring bone data from GLB files, providing an interactive UI to tweak physics parameters, live-previewing changes in the wearable preview, and patching the GLB binary on save.
Key changes:
Testing
Spring Bones Section:
Preview:
Body shapes:
Screenshots
Screen.Recording.2026-04-07.at.10.43.01.AM.mp4
Pending tasks