refactor: extract usePathtracingMesh hook and optimize Zustand selectors #865
Conversation
- Create usePathtracingMesh hook to encapsulate pathtracing mesh conversion logic, eliminating ~40 lines of duplicate code from 6 geometry components - Refactor Particles, Bonds, Arrow, Box, Plane, Shape to use the new hook - Fix type error in Bonds.tsx (hoverMeshRef was Mesh instead of InstancedMesh) - Optimize Zustand store access by using individual selectors instead of object destructuring to prevent unnecessary re-renders - Improve convertInstancedMesh utility with better bounding box computation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughRefactors pathtracing merged-mesh workflow into a new hook, reorganizes Canvas component categories (adds data+pathtracing and data-only sets, moves Curve/Cell/Floor), adds pathtracing guards and memoization to geometry components, replaces bulk store destructurings with per-property selectors, and defers pathtracer.update to useFrame. Changes
Sequence Diagram(s)mermaid Component->>Hook: call usePathtracingMesh(parentGroupRef, mainMeshRef, pathtracingEnabled) style Component fill:rgba(99,102,241,0.5) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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: 1
🤖 Fix all issues with AI agents
In `@app/src/utils/convertInstancedMesh.ts`:
- Around line 51-67: The merged index buffer currently chooses Uint16Array vs
Uint32Array based only on baseIndices.array type; change the logic in
convertInstancedMesh (around variables baseIndexCount/totalVertexCount and
mergedIndices) to also promote to Uint32Array when totalVertexCount > 65535 (or
totalVertexCount + base vertex offset could exceed 65535) so indices won't
overflow; ensure the condition checks (baseIndices?.array instanceof
Uint32Array) || totalVertexCount > 65535 and use Uint32Array in that case,
otherwise use Uint16Array, keeping mergedIndices creation consistent with
existing baseIndices handling.
🧹 Nitpick comments (8)
app/src/components/three/MultiGeometryTransformControls.tsx (1)
129-131: The eslint-disable is justified, but consider a more targeted disable.The comment correctly explains that
showSnackbarandupdateSelectionForGeometryare stable Zustand actions. However, a more targeted disable would be clearer:- // Note: showSnackbar and updateSelectionForGeometry are stable Zustand actions, omit from deps - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [mode, geometries, selections, loadedPositionsMap]); + }, [ + mode, + geometries, + selections, + loadedPositionsMap, + // showSnackbar and updateSelectionForGeometry are stable Zustand actions + // eslint-disable-next-line react-hooks/exhaustive-deps + ]);Alternatively, you could include them in the dependency array since they're stable anyway—it would satisfy the linter without causing additional re-renders.
app/src/hooks/useSocketManager.ts (1)
873-904: Dependency array is correct but extensive.All setters listed are stable Zustand references, so this large dependency array won't trigger spurious re-runs. However, consider extracting the socket event handlers into a custom hook or grouping related setters if this file grows further to improve maintainability.
app/src/components/three/Plane.tsx (1)
509-513: Consider addingupdateMergedMeshto the dependency array.While
updateMergedMeshis auseCallback-memoized function, the ESLintreact-hooks/exhaustive-depsrule typically expects all referenced functions to be included in the dependency array for correctness. Since it's stable, adding it won't cause extra re-runs but ensures compliance with React best practices.♻️ Suggested change
], [ frameCount, // Watch frameCount to clear planes when it becomes 0 isFetching, hasQueryError, positionData, sizeData, colorData, rotationData, scaleData, // Add scaleData positionProp, sizeProp, colorProp, rotationProp, scale, // Add scale instanceCount, validSelectedIndices, selecting, geometryKey, pathtracingEnabled, material, opacity, data, + updateMergedMesh, ]);app/src/components/three/Box.tsx (1)
505-509: Same optional suggestion: consider addingupdateMergedMeshto dependencies.For consistency and React lint compliance, consider adding
updateMergedMeshto the dependency array as noted in Plane.tsx.app/src/components/three/Shape.tsx (1)
456-459: Same optional suggestion: consider addingupdateMergedMeshto dependencies.app/src/components/three/Bonds.tsx (1)
655-661: Same optional suggestion: consider addingupdateMergedMeshto dependencies.For consistency with React hooks best practices, consider adding
updateMergedMeshto the dependency array.app/src/components/three/Particles.tsx (1)
685-709: Dependency array includes unused variables.The dependencies
particleResolution,material,opacity, anddataare added but none of these are directly used within the effect body. The effect processes data throughfullData(already in scope via destructuring), not these raw props.
particleResolution→ derived fromfullData.resolution(line 179)material,opacity→ destructured fromfullData(lines 113-119)data→ already covered byfullDatawhich is memoized withdataas dependencyIf the intent is to re-trigger the effect when these change for pathtracing mesh updates,
fullDatashould be added as a dependency instead, or document why these specific raw props are needed.Consider this adjustment
], [ frameCount, // Watch frameCount to clear particles when it becomes 0 isFetching, hasQueryError, positionData, colorData, radiusData, scaleData, transformData, positionProp, colorProp, radiusProp, scale, positionIsTransform, radiusIsTransform, colorIsTransform, instanceCount, validSelectedIndices, selecting, pathtracingEnabled, - particleResolution, - material, - opacity, - data, + updateMergedMesh, + setInstanceCount, + setParticleCount, ]);app/src/components/three/Arrow.tsx (1)
496-518: Same dependency array concern as Particles.tsx.The dependencies
resolution,material,opacity, anddataare added but not directly used in the effect body:
resolution→ used viageometrywhich is memoized separately (line 555)material,opacity→ used in JSX render, not in this effectdata→ already covered by destructured props fromfullDataIf these are intended to trigger pathtracing mesh rebuilds when visual properties change, consider adding
updateMergedMeshto dependencies instead for clarity, or document the intent.Consider this adjustment
], [ frameCount, // Watch frameCount to clear arrows when it becomes 0 isFetching, positionData, directionData, colorData, radiusData, scaleData, positionProp, directionProp, color, radius, scale, instanceCount, validSelectedIndices, selecting, geometryKey, pathtracingEnabled, - resolution, - material, - opacity, - data, + updateMergedMesh, + setInstanceCount, ]);
|
Issue with BMIM FILE that with resolution larger 10, BF4 is not rendered. Changing resolution requires step update? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #865 +/- ##
==========================================
- Coverage 79.81% 79.78% -0.03%
==========================================
Files 165 165
Lines 20213 20213
==========================================
- Hits 16133 16127 -6
- Misses 4080 4086 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add updateMergedMesh to useEffect dependencies in all geometry components - Memoize fullData in Cell.tsx for consistency with other components - Remove unused BufferGeometryUtils import from convertInstancedMesh.ts Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/utils/convertInstancedMesh.ts`:
- Around line 111-124: The current code in convertInstancedMesh.ts transforms
normals using only the rotation part of the instance matrix (e/e[0..10]) which
breaks for non-uniform scale; replace that with an inverse-transpose normal
matrix: compute a Matrix3 normalMatrix from the instance Matrix4 (use
Matrix3.getNormalMatrix(instanceMatrix) or equivalent), apply that normalMatrix
to the base normal (nx,ny,nz) to get the transformed normal, then renormalize
the resulting vector before writing into mergedNormals[targetIdx..+2]; update
the code paths that reference mergedNormals, baseNormals and targetIdx to use
this normal-matrix + normalize flow.
Summary by CodeRabbit
Performance
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.