Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 22 additions & 16 deletions frontend/src/features/editor/EditorApp.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import { useEffect, useMemo, useRef, useState } from "react";
import { useEffect, useRef, useState } from "react";

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.

medium

The useMemo hook is required for the optimizations suggested in the component body to ensure reference stability and prevent unnecessary re-renders.

Suggested change
import { useEffect, useRef, useState } from "react";
import { useEffect, useMemo, useRef, useState } from "react";

import { useGLTF } from "@react-three/drei";
import { useParams } from "react-router-dom";
import { useQuery } from "@tanstack/react-query";
import { posthog } from "@/shared/analytics/posthog";

import useWallSessionQuery from "./utils/WallSessionQuery";
import { usePlacementStore } from "./store";
import type { SessionHoldInstance } from "./store";
import { useHandleLoadSession } from "./utils/HandleLoadSession";

import MainCanvas from "./components/MainCanvas";
Expand All @@ -16,6 +15,15 @@ import FileManager from "./components/FileManager";
import { useTranslation } from "react-i18next";
import Tutorial from "./components/Tutorial";

interface HoldInstance {
id: string;
hold_instance_id?: string;
hold_type?: {
glb_url?: string;
};
[key: string]: unknown;
}

const transformTools = [
{ id: "translate", icon: "open_with", label: "Translate", hint: "Shift + Left click" },
{ id: "rotate", icon: "sync", label: "Rotate", hint: "Left click" },
Expand Down Expand Up @@ -76,25 +84,23 @@ function EditorApp() {
return () => window.removeEventListener("beforeunload", handleBeforeUnload);
}, [hasUnsavedChanges, t]);

const holdModels: Array<Record<string, HoldInstance>> = [];
const holdModelsGLBURL: string[] = [];

useEffect(() => {
const glbUrl = session_data?.related_wall?.glb_url;
setWallModels(glbUrl ? [glbUrl] : []);
}, [session_data?.related_wall?.glb_url]);

const { holdModels, holdModelsGLBURL } = useMemo(() => {
const holdModels: SessionHoldInstance[] = [];
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);
}
});
}
return { holdModels, holdModelsGLBURL };
}, [session_data?.related_holds_collection, session_data?.holds_collection_instances]);
if (session_data?.related_holds_collection) {
session_data.holds_collection_instances?.forEach((hold: any) => {
hold.hold_instance_id = hold.id;
holdModels.push(hold);
if (hold.hold_type?.glb_url) {
holdModelsGLBURL.push(hold.hold_type.glb_url);
}
});
}
Comment on lines +87 to +103

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.

high

Calculating holdModels and holdModelsGLBURL directly in the render body introduces several issues:

  1. Performance: These arrays are recreated on every render. Since holdModelsGLBURL is a dependency for the useEffect on line 110 (preloading), that effect will now trigger on every render cycle.
  2. Side Effects in Render: The line hold.hold_instance_id = hold.id mutates the session_data object during rendering. If session_data is managed by a state library or cache (like TanStack Query), this mutation can lead to unpredictable UI bugs.
  3. Type Mismatch: The type Array<Record<string, HoldInstance>> on line 87 does not match the usage (it's being populated as a flat array of objects).

Wrapping this logic in useMemo is highly recommended.

Suggested change
const holdModels: Array<Record<string, HoldInstance>> = [];
const holdModelsGLBURL: string[] = [];
useEffect(() => {
const glbUrl = session_data?.related_wall?.glb_url;
setWallModels(glbUrl ? [glbUrl] : []);
}, [session_data?.related_wall?.glb_url]);
const { holdModels, holdModelsGLBURL } = useMemo(() => {
const holdModels: SessionHoldInstance[] = [];
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);
}
});
}
return { holdModels, holdModelsGLBURL };
}, [session_data?.related_holds_collection, session_data?.holds_collection_instances]);
if (session_data?.related_holds_collection) {
session_data.holds_collection_instances?.forEach((hold: any) => {
hold.hold_instance_id = hold.id;
holdModels.push(hold);
if (hold.hold_type?.glb_url) {
holdModelsGLBURL.push(hold.hold_type.glb_url);
}
});
}
const { holdModels, holdModelsGLBURL } = useMemo(() => {
const models: any[] = [];
const urls: string[] = [];
if (session_data?.related_holds_collection) {
session_data.holds_collection_instances?.forEach((hold: any) => {
const holdWithId = { ...hold, hold_instance_id: hold.id };
models.push(holdWithId);
if (hold.hold_type?.glb_url) {
urls.push(hold.hold_type.glb_url);
}
});
}
return { holdModels: models, holdModelsGLBURL: urls };
}, [session_data?.related_holds_collection, session_data?.holds_collection_instances]);
useEffect(() => {
const glbUrl = session_data?.related_wall?.glb_url;
setWallModels(glbUrl ? [glbUrl] : []);
}, [session_data?.related_wall?.glb_url]);


const { preload } = useGLTF;
useEffect(() => {
Expand Down
29 changes: 14 additions & 15 deletions frontend/src/features/editor/components/AddHoldModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,19 @@ import HandleAddHold from "../utils/HandleAddHold";
import { useEditorAuth } from "../mocks/useEditorAuth";
import PaginationList from "../stubs/PaginationList";
import { useTranslation } from "react-i18next";
import type { HoldModel, SessionData } from "../store";

type StockItem = {
id: string | number;
hold_type: { id: string | number; cdn_ref?: string; [key: string]: unknown };
model?: string;
[key: string]: unknown;
type HoldModel = {
name: string;
file: string;
hold_type: Record<string, any>;
hold_instance_id: string;
id?: string;
};

interface AddHoldModalProps {
isOpen: boolean;
onClose: () => void;
session_data: SessionData;
session_data: any;
onHoldAdded: (hold: HoldModel) => void;
currentHolds: HoldModel[];
}
Expand All @@ -32,7 +32,7 @@ export default function AddHoldModal({
const [search, setSearch] = useState("");
const [isAdding, setIsAdding] = useState(false);
const scrollRef = useRef<HTMLElement>(null);
const [addedHoldIds, setAddedHoldIds] = useState<Set<string | number>>(new Set());
const [addedHoldIds, setAddedHoldIds] = useState<Set<number>>(new Set());
const { user, authenticatedFetch } = useEditorAuth();
const [page, setPage] = useState(1);
const API_URL = import.meta.env.VITE_API_BASE;
Expand Down Expand Up @@ -97,7 +97,7 @@ export default function AddHoldModal({
);

const deduplicatedStockData = Array.isArray(rawStockData)
? rawStockData.reduce((acc: StockItem[], current: StockItem) => {
? rawStockData.reduce((acc: any[], current: any) => {
const exists = acc.find(
(hold) => hold.hold_type.id === current.hold_type.id
);
Expand All @@ -108,25 +108,24 @@ export default function AddHoldModal({

const stockData = Array.isArray(deduplicatedStockData)
? deduplicatedStockData.filter(
(hold: StockItem) =>
(hold: any) =>
!currentHoldTypeIds.has(hold.hold_type.id) &&
!addedHoldIds.has(hold.hold_type.id as number)
!addedHoldIds.has(hold.hold_type.id)
)
: deduplicatedStockData;

const filteredStockData = Array.isArray(stockData)
? stockData.filter((hold: StockItem) => {
? stockData.filter((hold: any) => {
if (!search) return true;
const searchLower = search.toLowerCase();
const manufacturerName = hold.hold_type?.manufacturer_name as string | undefined;
return (
hold.model?.toLowerCase().includes(searchLower) ||
manufacturerName?.toLowerCase().includes(searchLower)
hold.hold_type?.manufacturer_name?.toLowerCase().includes(searchLower)
);
})
: stockData;

const handleAddHoldClick = async (hold: StockItem) => {
const handleAddHoldClick = async (hold: any) => {
setIsAdding(true);
try {
const result = await HandleAddHold(hold, session_data, onHoldAdded);
Expand Down
12 changes: 5 additions & 7 deletions frontend/src/features/editor/components/FileManager.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import { useState } from "react";
import { usePlacementStore } from "../store";
import type { SessionData } from "../store";
import { useNavigate } from "react-router-dom";
import { useEditorAuth } from "../mocks/useEditorAuth";
import { useTranslation } from "react-i18next";
import { posthog } from "@/shared/analytics/posthog";

const FileManager = ({ session_data }: { session_data: SessionData }) => {
const FileManager = ({ session_data }: { session_data: any }) => {
const objects = usePlacementStore((s) => s.objects);
const wallColors = usePlacementStore((s) => s.wallColors);
const holdColors = usePlacementStore((s) => s.holdColors);
Expand All @@ -17,16 +16,15 @@ const FileManager = ({ session_data }: { session_data: SessionData }) => {
const navigate = useNavigate();
const { t } = useTranslation();

const cleanObjectsForSave = (objs: typeof objects, data: SessionData) => {
const cleanObjectsForSave = (objs: typeof objects, data: any) => {
return objs.map((obj) => {
const cleanedObj: Record<string, unknown> = { ...obj };
const url = cleanedObj.url as string | undefined;
if (cleanedObj.type === "wall" && url?.startsWith("blob:")) {
const cleanedObj = { ...obj } as any;
if (cleanedObj.type === "wall" && cleanedObj.url?.startsWith("blob:")) {
if (data?.related_wall?.id) {
cleanedObj.wall_id = data.related_wall.id;
}
delete cleanedObj.url;
} else if (url?.startsWith("blob:")) {
} else if (cleanedObj.url?.startsWith("blob:")) {
delete cleanedObj.url;
}
return cleanedObj;
Expand Down
8 changes: 4 additions & 4 deletions frontend/src/features/editor/components/HoldInspector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ function RotationHandle({
const handleY = radius + handleRadius * Math.sin(rotation - Math.PI / 2);
const onPointerDown = (e: React.PointerEvent) => {
e.preventDefault();
window.addEventListener("pointermove", onPointerMove as EventListener);
window.addEventListener("pointerup", onPointerUp as EventListener);
window.addEventListener("pointermove", onPointerMove as any);
window.addEventListener("pointerup", onPointerUp as any);
};
const onPointerMove = (e: PointerEvent) => {
if (!circleRef.current) return;
Expand All @@ -29,8 +29,8 @@ function RotationHandle({
onRotate(angle);
};
const onPointerUp = () => {
window.removeEventListener("pointermove", onPointerMove as EventListener);
window.removeEventListener("pointerup", onPointerUp as EventListener);
window.removeEventListener("pointermove", onPointerMove as any);
window.removeEventListener("pointerup", onPointerUp as any);
};
return (
<div
Expand Down
Loading
Loading