refactor(EditorApp, AddHoldModal): improve hold instance handling and…#22
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors parts of the editor frontend to improve type safety and avoid mutating hold instance objects derived from session data.
Changes:
- Remove an unnecessary
as numbercast when checkingaddedHoldIdsinAddHoldModal. - Avoid mutating
SessionHoldInstanceobjects inEditorAppby creating a derived object withhold_instance_id.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| frontend/src/features/editor/components/AddHoldModal.tsx | Uses the native `string |
| frontend/src/features/editor/EditorApp.tsx | Stops mutating session hold instances by pushing a cloned object with hold_instance_id set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request refactors the hold model processing in EditorApp.tsx to use immutable object updates and removes an unnecessary type assertion in AddHoldModal.tsx. Feedback suggests using a Set to deduplicate GLB URLs in EditorApp.tsx to avoid redundant processing and improve efficiency.
| const holdModelsGLBURL: string[] = []; | ||
| if (session_data?.related_holds_collection) { | ||
| session_data.holds_collection_instances?.forEach((hold: SessionHoldInstance) => { | ||
| hold.hold_instance_id = hold.id; | ||
| holdModels.push(hold); | ||
| if (hold.hold_type?.glb_url) { | ||
| holdModelsGLBURL.push(hold.hold_type.glb_url); | ||
| const holdWithId = { ...hold, hold_instance_id: hold.id }; | ||
| holdModels.push(holdWithId); | ||
| if (holdWithId.hold_type?.glb_url) { | ||
| holdModelsGLBURL.push(holdWithId.hold_type.glb_url); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The holdModelsGLBURL array can accumulate many duplicate URLs if multiple hold instances share the same type. While useGLTF.preload likely handles duplicates internally, deduplicating the list here avoids redundant iterations and function calls in the useEffect at line 100. Using a Set is a more efficient way to collect unique URLs.
| const holdModelsGLBURL: string[] = []; | |
| if (session_data?.related_holds_collection) { | |
| session_data.holds_collection_instances?.forEach((hold: SessionHoldInstance) => { | |
| hold.hold_instance_id = hold.id; | |
| holdModels.push(hold); | |
| if (hold.hold_type?.glb_url) { | |
| holdModelsGLBURL.push(hold.hold_type.glb_url); | |
| const holdWithId = { ...hold, hold_instance_id: hold.id }; | |
| holdModels.push(holdWithId); | |
| if (holdWithId.hold_type?.glb_url) { | |
| holdModelsGLBURL.push(holdWithId.hold_type.glb_url); | |
| } | |
| }); | |
| } | |
| const glbUrlSet = new Set<string>(); | |
| if (session_data?.related_holds_collection) { | |
| session_data.holds_collection_instances?.forEach((hold: SessionHoldInstance) => { | |
| const holdWithId = { ...hold, hold_instance_id: hold.id }; | |
| holdModels.push(holdWithId); | |
| if (holdWithId.hold_type?.glb_url) { | |
| glbUrlSet.add(holdWithId.hold_type.glb_url); | |
| } | |
| }); | |
| } | |
| const holdModelsGLBURL = Array.from(glbUrlSet); |
… type safety