From 7d4c8b6bd4feceda4502ea95b2f0e4915ede59a0 Mon Sep 17 00:00:00 2001 From: Qiaochu Hu <110hqc@gmail.com> Date: Thu, 26 Mar 2026 12:30:16 +0800 Subject: [PATCH] fix: resolve state management issues and memory leaks (#152) This commit fixes several issues in the core state management: 1. Fix deleteNodesAction recursive call issue - Previously, the function recursively called get().deleteNodes() inside the set() callback, which could cause state inconsistencies. - Now collects all descendant IDs first in a separate pass, then deletes them all in a single batch operation. 2. Fix memory leak in use-scene temporal tracking - Module-level variables (prevPastLength, prevFutureLength, prevNodesSnapshot) persisted across store re-creations. - Now cleared in unloadScene() to release stale node references. - Added clearTemporalTracking() helper function. 3. Remove unused variable in wall-system - Removed debug variable 'useFrameNb' that was declared but never used. 4. Fix requestAnimationFrame in updateNodesAction - Previously used RAF without cleanup, causing multiple queued callbacks when updates happened rapidly. - Now uses a single tracked RAF with cancellation support. Co-authored-by: hobostay Co-authored-by: Claude Opus 4.6 (1M context) --- .../core/src/store/actions/node-actions.ts | 55 ++++++++++++++----- packages/core/src/store/use-scene.ts | 17 +++++- .../core/src/systems/wall/wall-system.tsx | 2 - 3 files changed, 57 insertions(+), 17 deletions(-) diff --git a/packages/core/src/store/actions/node-actions.ts b/packages/core/src/store/actions/node-actions.ts index d05ec0ca..e162a873 100644 --- a/packages/core/src/store/actions/node-actions.ts +++ b/packages/core/src/store/actions/node-actions.ts @@ -4,6 +4,10 @@ import type { SceneState } from '../use-scene' type AnyContainerNode = AnyNode & { children: string[] } +// Track pending RAF for updateNodesAction to prevent multiple queued callbacks +let pendingRafId: number | null = null +let pendingUpdates: Set = new Set() + export const createNodesAction = ( set: (fn: (state: SceneState) => Partial) => void, get: () => SceneState, @@ -58,6 +62,7 @@ export const updateNodesAction = ( updates: { id: AnyNodeId; data: Partial }[], ) => { const parentsToUpdate = new Set() + const idsToMarkDirty = new Set() set((state) => { const nextNodes = { ...state.nodes } @@ -98,14 +103,25 @@ export const updateNodesAction = ( return { nodes: nextNodes } }) - // Mark dirty after the next frame to ensure React renders complete - requestAnimationFrame(() => { - updates.forEach((u) => { - get().markDirty(u.id) - }) - parentsToUpdate.forEach((pId) => { - get().markDirty(pId) + // Collect all IDs that need to be marked dirty + updates.forEach((u) => idsToMarkDirty.add(u.id)) + parentsToUpdate.forEach((pId) => idsToMarkDirty.add(pId)) + + // Add to pending updates set + idsToMarkDirty.forEach((id) => pendingUpdates.add(id)) + + // Cancel any pending RAF and schedule a new one + if (pendingRafId !== null) { + cancelAnimationFrame(pendingRafId) + } + + pendingRafId = requestAnimationFrame(() => { + // Mark all pending updates as dirty + pendingUpdates.forEach((id) => { + get().markDirty(id) }) + pendingUpdates.clear() + pendingRafId = null }) } @@ -121,7 +137,26 @@ export const deleteNodesAction = ( const nextCollections = { ...state.collections } let nextRootIds = [...state.rootNodeIds] + // Collect all IDs to delete (including descendants) in a first pass + // This avoids issues with recursive calls during state mutation + const allIdsToDelete = new Set() + const collectDescendants = (id: AnyNodeId) => { + const node = nextNodes[id] + if (!node) return + allIdsToDelete.add(id) + if ('children' in node && node.children) { + for (const childId of node.children as AnyNodeId[]) { + collectDescendants(childId) + } + } + } + for (const id of ids) { + collectDescendants(id) + } + + // Now process all nodes for deletion + for (const id of allIdsToDelete) { const node = nextNodes[id] if (!node) continue @@ -153,12 +188,6 @@ export const deleteNodesAction = ( // 4. Delete the node itself delete nextNodes[id] - - // Inside the deleteNodes loop - if ('children' in node && node.children.length > 0) { - // Recursively delete all children first - get().deleteNodes(node.children as AnyNodeId[]) - } } return { nodes: nextNodes, rootNodeIds: nextRootIds, collections: nextCollections } diff --git a/packages/core/src/store/use-scene.ts b/packages/core/src/store/use-scene.ts index e3773a59..b8860939 100644 --- a/packages/core/src/store/use-scene.ts +++ b/packages/core/src/store/use-scene.ts @@ -115,6 +115,11 @@ const useScene: UseSceneStore = create()( collections: {} as Record, unloadScene: () => { + // Clear temporal tracking to prevent memory leaks from stale node references + prevPastLength = 0 + prevFutureLength = 0 + prevNodesSnapshot = null + set({ nodes: {}, rootNodeIds: [], @@ -305,13 +310,21 @@ let prevPastLength = 0 let prevFutureLength = 0 let prevNodesSnapshot: Record | null = null -export function clearSceneHistory() { - useScene.temporal.getState().clear() +/** + * Clears temporal history tracking variables to prevent memory leaks. + * Should be called when unloading a scene to release node references. + */ +export function clearTemporalTracking() { prevPastLength = 0 prevFutureLength = 0 prevNodesSnapshot = null } +export function clearSceneHistory() { + useScene.temporal.getState().clear() + clearTemporalTracking() +} + // Subscribe to the temporal store (Undo/Redo events) useScene.temporal.subscribe((state) => { const currentPastLength = state.pastStates.length diff --git a/packages/core/src/systems/wall/wall-system.tsx b/packages/core/src/systems/wall/wall-system.tsx index 50c60e4b..2147edc1 100644 --- a/packages/core/src/systems/wall/wall-system.tsx +++ b/packages/core/src/systems/wall/wall-system.tsx @@ -22,7 +22,6 @@ const csgEvaluator = new Evaluator() // WALL SYSTEM // ============================================================================ -let useFrameNb = 0 export const WallSystem = () => { const dirtyNodes = useScene((state) => state.dirtyNodes) const clearDirty = useScene((state) => state.clearDirty) @@ -35,7 +34,6 @@ export const WallSystem = () => { // Collect dirty walls and their levels const dirtyWallsByLevel = new Map>() - useFrameNb += 1 dirtyNodes.forEach((id) => { const node = nodes[id] if (!node || node.type !== 'wall') return