Dirty-lifecycle correctness: guarded dirty set, undo phantom sweep, animation split

.agents/skills/review-architecture/SKILL.md

@@ -121,7 +121,7 @@ If the PR adds or modifies a node kind, check against `wiki/architecture/node-de
 - New state added to `useViewer` must be presentation-only (selection, camera, level mode, display toggles). Editor-only state (active tool, phase, edit mode, paint preview, floorplan state) goes in `useEditor`.
 - **Node code does not import `useScene` directly.** A kind's geometry / system / tool should read and write through `SceneApi` (passed in by the framework) or `GeometryContext`. Direct `useScene.getState()` calls inside `packages/nodes/src/<kind>/` are a smell — they bypass the registry's IoC point and make the code harder to test.
 - **Live drag motion is imperative, not store-driven.** Tools must not call `useLiveTransforms.set(...)` per `grid:move` tick to animate registered parametric kinds — the selector path doesn't reliably re-render and the mesh visibly disappears mid-drag. Use `sceneRegistry.nodes.get(node.id)?.position.set(x, y, z)` instead, and commit once at the end via `useScene.temporal.getState().resume() → updateNode → pause()`. The reference implementation is `MoveRegistryNodeTool`. This is the *only* sanctioned use of imperative mesh transforms by a tool; flag any other location that does the same.
-- **Data-driven drags preview via `useLiveNodeOverrides`, never per-tick `useScene`.** A kind whose geometry is recomputed from data fields (wall `start`/`end`, opening host-cut, endpoint reshape) previews by publishing field patches to `useLiveNodeOverrides` (merged by `getEffectiveWall` / `getEffectiveNode`), writing the scene store **once on commit**. A tool that calls `useScene.updateNodes`/`updateNode` on `grid:move` (or any per-pointer-move tick) is a **blocker** — it swaps the `nodes` map ref and re-renders every `useScene(s => s.nodes)` subscriber app-wide each frame (`markDirty` per tick is fine). Grep tell: `updateNode(s)?(` in an `onGridMove`/`onMove`/`applyPreview` path under `packages/nodes/src/<kind>/`. See `wiki/architecture/tools.md` § "Data-driven live drag".
+- **Data-driven drags preview via `useLiveNodeOverrides`, never per-tick `useScene`.** A kind whose geometry is recomputed from data fields (wall `start`/`end`, opening host-cut, endpoint reshape) previews by publishing field patches to `useLiveNodeOverrides` (merged by `getEffectiveWall` / `getEffectiveNode`), writing the scene store **once on commit**. A tool that calls `useScene.updateNodes`/`updateNode` on `grid:move` (or any per-pointer-move tick) is a **blocker** — it swaps the `nodes` map ref and re-renders every `useScene(s => s.nodes)` subscriber app-wide each frame. `markDirty` per tick is fine **for bounded gestures** (drag marks drain every frame); a `useFrame`/animation loop that marks dirty for as long as something animates is a **blocker** — the scene can then never settle to DIRTY 0. Animations signal rebuilds through their own records (`useInteractive` animations), marking dirty once on completion; see `wiki/architecture/node-definitions.md` § "`geometry` + `system`". Grep tell: `updateNode(s)?(` in an `onGridMove`/`onMove`/`applyPreview` path under `packages/nodes/src/<kind>/`. See `wiki/architecture/tools.md` § "Data-driven live drag".
 
 ### D. Selector performance
 

packages/core/src/store/use-scene-dirty-tracking.test.ts

@@ -39,14 +39,16 @@ describe('dirty tracking', () => {
   beforeEach(() => {
     if (!nodeRegistry.has(untrackedDef.kind)) nodeRegistry._register(untrackedDef)
     if (!nodeRegistry.has(trackedDef.kind)) nodeRegistry._register(trackedDef)
+    // Clear rather than replace the dirty set: the store's own instance is the
+    // guarded one, and the raw-add tests below exercise that guard.
+    useScene.getState().dirtyNodes.clear()
     useScene.setState({
       nodes: {
         [UNTRACKED]: makeNode(UNTRACKED, 'test-untracked'),
         [TRACKED]: makeNode(TRACKED, 'test-tracked'),
         [UNREGISTERED]: makeNode(UNREGISTERED, 'unregistered-kind'),
       },
       rootNodeIds: [UNTRACKED, TRACKED, UNREGISTERED],
-      dirtyNodes: new Set(),
       collections: {},
     } as never)
     useScene.temporal.getState().clear()
@@ -67,6 +69,36 @@ describe('dirty tracking', () => {
     expect(useScene.getState().dirtyNodes.has(UNREGISTERED)).toBe(true)
   })
 
+  test('raw dirtyNodes.add applies the same consumer-kind guard as markDirty', () => {
+    useScene.getState().dirtyNodes.add(UNTRACKED)
+    useScene.getState().dirtyNodes.add(TRACKED)
+    expect(useScene.getState().dirtyNodes.has(UNTRACKED)).toBe(false)
+    expect(useScene.getState().dirtyNodes.has(TRACKED)).toBe(true)
+  })
+
+  test('raw dirtyNodes.add accepts ids with no node yet', () => {
+    const pending = 'item_pending_create' as AnyNodeId
+    useScene.getState().dirtyNodes.add(pending)
+    expect(useScene.getState().dirtyNodes.has(pending)).toBe(true)
+  })
+
+  test('undo clears dirty marks whose node no longer exists', async () => {
+    const NEW = 'item_undone_away' as AnyNodeId
+    // Tracked write: pushes the pre-write state (without NEW) onto pastStates.
+    useScene.setState({
+      nodes: { ...useScene.getState().nodes, [NEW]: makeNode(NEW, 'test-tracked') },
+    } as never)
+    useScene.getState().markDirty(NEW)
+    expect(useScene.getState().dirtyNodes.has(NEW)).toBe(true)
+
+    useScene.temporal.getState().undo()
+    // The sweep runs in the temporal subscriber's microtask.
+    await new Promise((resolve) => setTimeout(resolve, 0))
+
+    expect(useScene.getState().nodes[NEW]).toBeUndefined()
+    expect(useScene.getState().dirtyNodes.has(NEW)).toBe(false)
+  })
+
   test('deleteNodes removes deleted ids from the dirty set', () => {
     useScene.getState().markDirty(TRACKED)
     expect(useScene.getState().dirtyNodes.has(TRACKED)).toBe(true)

packages/core/src/store/use-scene.ts

@@ -1342,6 +1342,47 @@ function sceneHistorySnapshotFromState(
   }
 }
 
+/**
+ * A dirty mark is a promise that some system will rebuild the node and clear
+ * the mark, so marks are only accepted for kinds with a dirty consumer: kinds
+ * with `dirtyTracking: false` (and kinds of disabled plugins) have none, and
+ * a mark for them would sit in the set for the whole session and defeat every
+ * consumer's empty-set early exit. Ids without a node pass: tools mark nodes
+ * they are about to create.
+ */
+function isDirtyTrackable(
+  id: AnyNodeId,
+  scene: Pick<SceneState, 'nodes' | 'installedPlugins'>,
+): boolean {
+  const node = scene.nodes[id]
+  if (!node) return true
+  if (!isNodeKindEnabled(node.type, scene.installedPlugins)) return false
+  return nodeRegistry.get(node.type)?.dirtyTracking !== false
+}
+
+/**
+ * `markDirty` always applied the consumer-kind guard, but many call sites add
+ * to the raw set directly (that is how stuck `level` marks got in) — enforcing
+ * it in `add` itself keeps them all honest.
+ */
+class GuardedDirtySet extends Set<AnyNodeId> {
+  private readonly getScene: () => Pick<SceneState, 'nodes' | 'installedPlugins'>
+
+  constructor(
+    getScene: () => Pick<SceneState, 'nodes' | 'installedPlugins'>,
+    from?: Iterable<AnyNodeId>,
+  ) {
+    super()
+    this.getScene = getScene
+    if (from) for (const id of from) this.add(id)
+  }
+
+  override add(id: AnyNodeId): this {
+    if (!isDirtyTrackable(id, this.getScene())) return this
+    return super.add(id)
+  }
+}
+
 const useScene: UseSceneStore = create<SceneState>()(
   temporal(
     (set, get) => ({
@@ -1352,7 +1393,7 @@ const useScene: UseSceneStore = create<SceneState>()(
       rootNodeIds: [],
 
       // 3. Dirty set
-      dirtyNodes: new Set<AnyNodeId>(),
+      dirtyNodes: new GuardedDirtySet(get),
 
       // 4. Collections
       collections: {} as Record<CollectionId, Collection>,
@@ -1368,7 +1409,7 @@ const useScene: UseSceneStore = create<SceneState>()(
         set({
           nodes: {},
           rootNodeIds: [],
-          dirtyNodes: new Set<AnyNodeId>(),
+          dirtyNodes: new GuardedDirtySet(get),
           collections: {},
           materials: {},
           installedPlugins: [],
@@ -1424,7 +1465,7 @@ const useScene: UseSceneStore = create<SceneState>()(
         set({
           nodes: cleanedNodes,
           rootNodeIds: normalizedRootNodeIds,
-          dirtyNodes: new Set<AnyNodeId>(),
+          dirtyNodes: new GuardedDirtySet(get),
           collections: extra?.collections ?? {},
           materials,
           installedPlugins: Array.from(new Set(extra?.installedPlugins ?? [])),
@@ -1440,7 +1481,12 @@ const useScene: UseSceneStore = create<SceneState>()(
         if (get().readOnly) return
         const nextInstalledPlugins = Array.from(new Set(pluginIds))
         const previousInstalledPlugins = get().installedPlugins
-        const dirtyNodes = new Set(get().dirtyNodes)
+        // Guard against the *next* plugin list: the store still holds the old
+        // one, and re-marks for newly enabled kinds must pass the guard.
+        const dirtyNodes = new GuardedDirtySet(
+          () => ({ nodes: get().nodes, installedPlugins: nextInstalledPlugins }),
+          get().dirtyNodes,
+        )
         for (const node of Object.values(get().nodes)) {
           if (!getNodePluginId(node.type)) continue
           if (!isNodeKindEnabled(node.type, nextInstalledPlugins)) {
@@ -1494,9 +1540,9 @@ const useScene: UseSceneStore = create<SceneState>()(
       },
 
       markDirty: (id) => {
-        const node = get().nodes[id]
-        if (node && !isNodeKindEnabled(node.type, get().installedPlugins)) return
-        if (node && nodeRegistry.get(node.type)?.dirtyTracking === false) return
+        // Guarded here too, not just in GuardedDirtySet.add — tests (and any
+        // setState caller) can swap in a plain Set.
+        if (!isDirtyTrackable(id, get())) return
         get().dirtyNodes.add(id)
       },
 
@@ -2163,6 +2209,14 @@ useScene.temporal.subscribe((state) => {
           markDirty(node.id)
         }
       }
+
+      // Undo/redo rewrites `nodes` without going through the delete actions,
+      // so marks for nodes that no longer exist would sit in the set for the
+      // rest of the session — no system clears a mark whose node is gone.
+      const { dirtyNodes, clearDirty } = useScene.getState()
+      for (const id of [...dirtyNodes]) {
+        if (!currentNodes[id]) clearDirty(id)
+      }
     })
   }
 

packages/viewer/src/components/viewer/perf-monitor.tsx

@@ -79,6 +79,25 @@ export const PerfMonitor = () => {
           .filter((n) => n.type === type)
           .map((n) => n.id as string)
       },
+      // Raw dirty-set census: total marks, marks whose node is gone (phantoms),
+      // and live marks bucketed by node kind. The panel's DIRTY readout filters
+      // to live nodes, so scripted runs need this to see leaks at all.
+      dirtyResidue(): {
+        total: number
+        phantom: number
+        phantomIds: string[]
+        liveByType: Record<string, number>
+      } {
+        const { dirtyNodes, nodes } = useScene.getState()
+        const phantomIds: string[] = []
+        const liveByType: Record<string, number> = {}
+        for (const id of dirtyNodes) {
+          const node = nodes[id]
+          if (!node) phantomIds.push(id as string)
+          else liveByType[node.type] = (liveByType[node.type] ?? 0) + 1
+        }
+        return { total: dirtyNodes.size, phantom: phantomIds.length, phantomIds, liveByType }
+      },
       projectNode(nodeId: string): { x: number; y: number; behindCamera: boolean } | null {
         const object = sceneRegistry.nodes.get(nodeId)
         if (!object) return null

packages/viewer/src/systems/door/door-animation-system.tsx

@@ -3,13 +3,6 @@ import { useFrame } from '@react-three/fiber'
 
 const easeDoorAnimation = (value: number) => value * value * (3 - 2 * value)
 
-function markDoorDirty(doorId: AnyNodeId) {
-  const scene = useScene.getState()
-  const node = scene.nodes[doorId]
-  scene.dirtyNodes.add(doorId)
-  if (node?.parentId) scene.dirtyNodes.add(node.parentId as AnyNodeId)
-}
-
 export const DoorAnimationSystem = () => {
   useFrame(({ clock }) => {
     const interactive = useInteractive.getState()
@@ -35,19 +28,23 @@ export const DoorAnimationSystem = () => {
 
       const progress = Math.min(1, (now - startedAt) / animation.durationMs)
       const value = animation.from + (animation.to - animation.from) * easeDoorAnimation(progress)
+      // No dirty mark per tick: DoorSystem rebuilds any door with an entry in
+      // `doorAnimations`, and a dirty mark is a one-shot work item, not a
+      // needs-frame signal — per-tick marks kept the scene from ever settling.
       interactive.setDoorOpenState(typedDoorId, { [animation.field]: value })
-      markDoorDirty(typedDoorId)
 
       if (progress < 1) continue
 
       interactive.cancelDoorAnimation(typedDoorId)
       if (animation.persist) {
         scene.updateNode(typedDoorId, { [animation.field]: animation.to })
         interactive.removeDoorOpenState(typedDoorId)
-        markDoorDirty(typedDoorId)
       } else {
         interactive.setDoorOpenState(typedDoorId, { [animation.field]: animation.to })
       }
+      // One final mark so the settled pose gets a rebuild after the animation
+      // entry is gone (the persist branch's updateNode also marks, harmlessly).
+      scene.markDirty(typedDoorId)
       emitter.emit('door:animation-completed', {
         doorId: typedDoorId as DoorNode['id'],
         field: animation.field,

packages/viewer/src/systems/door/door-system.tsx

@@ -123,7 +123,11 @@ export const DoorSystem = () => {
   }, [sceneMaterials])
 
   useFrame(() => {
-    if (dirtyNodes.size === 0) return
+    // Doors mid-swing rebuild every tick via their `doorAnimations` entry —
+    // the tween is a needs-frame signal, not dirty-set work (the set must be
+    // able to reach zero while an animation runs).
+    const animatingDoorIds = Object.keys(useInteractive.getState().doorAnimations) as AnyNodeId[]
+    if (dirtyNodes.size === 0 && animatingDoorIds.length === 0) return
     const frameJoineryMaterial = createSurfaceRoleMaterial('joinery', colorPreset)
     baseMaterial = textures ? getBaseMaterial(shading) : frameJoineryMaterial
     frameMaterial = textures ? getBaseMaterial(shading) : frameJoineryMaterial
@@ -143,6 +147,9 @@ export const DoorSystem = () => {
       if (node?.type !== 'door') return
       dirtyDoorIds.push(id as AnyNodeId)
     })
+    for (const id of animatingDoorIds) {
+      if (nodes[id]?.type === 'door' && !dirtyDoorIds.includes(id)) dirtyDoorIds.push(id)
+    }
 
     const useProgressiveDoorRebuilds = dirtyDoorIds.length > DOOR_PROGRESSIVE_DIRTY_THRESHOLD
     const frameStartedAt = performance.now()

packages/viewer/src/systems/perf-action-settle/perf-action-settle-system.tsx

@@ -11,17 +11,12 @@ const SETTLE_PRIORITY = 100
 
 const PerfActionSettleFrame = () => {
   useFrame(() => {
-    // Count only dirty marks whose node still exists. A node deleted while
-    // dirty (undo of a wall split, redo storms) leaves its mark in dirtyNodes
-    // forever — no system clears marks for missing nodes — and that phantom
-    // dirt would keep every action from ever settling. Real finding, tracked
-    // in plans/performance/editor-scalable-scene-runtime.md.
-    const { dirtyNodes, nodes } = useScene.getState()
-    let liveDirty = 0
-    dirtyNodes.forEach((id) => {
-      if (nodes[id]) liveDirty++
-    })
-    notifyPerfActionFrame(liveDirty, getPendingWallRebuildCount())
+    // The raw set size, deliberately: the dirty lifecycle now guarantees marks
+    // are cleared when their node goes away (undo sweep) and never added for
+    // consumerless kinds (GuardedDirtySet), so any lingering mark is a leak
+    // that SHOULD fail settle instead of being filtered out here.
+    const { dirtyNodes } = useScene.getState()
+    notifyPerfActionFrame(dirtyNodes.size, getPendingWallRebuildCount())
   }, SETTLE_PRIORITY)
   return null
 }

packages/viewer/src/systems/window/window-animation-system.tsx

@@ -17,18 +17,13 @@ import {
   FRENCH_CASEMENT_RIGHT_SASH_NAME,
   HOPPER_WINDOW_SASH_NAME,
   LOUVERED_WINDOW_SLATS_NAME,
+  pendingWindowAnimationRebuilds,
   SINGLE_HUNG_ACTIVE_SASH_NAME,
   SLIDING_WINDOW_ACTIVE_PANEL_NAME,
 } from './window-system'
 
 const easeWindowAnimation = (value: number) => value * value * (3 - 2 * value)
 
-function markWindowDirty(windowId: AnyNodeId) {
-  const scene = useScene.getState()
-  const node = scene.nodes[windowId]
-  scene.dirtyNodes.add(windowId)
-}
-
 /**
  * Pose a window's moving parts (sash/panel/slats) at `value` (0 = closed,
  * 1 = open) by mutating the named child groups under `mesh`. Returns true when
@@ -162,17 +157,22 @@ export const WindowAnimationSystem = () => {
       const value = animation.from + (animation.to - animation.from) * easeWindowAnimation(progress)
       interactive.setWindowOpenState(typedWindowId, { [animation.field]: value })
       const appliedDirectly = applyDirectWindowAnimation(typedWindowId, value)
-      if (!appliedDirectly) markWindowDirty(typedWindowId)
+      // A dirty mark is one-shot work, not a needs-frame signal — per-tick
+      // marks kept the scene from ever settling. Types without a direct pose
+      // path get a transient rebuild request instead.
+      if (!appliedDirectly) pendingWindowAnimationRebuilds.add(typedWindowId)
 
       if (progress < 1) continue
 
       interactive.cancelWindowAnimation(typedWindowId)
       if (animation.persist) {
         scene.updateNode(typedWindowId, { [animation.field]: animation.to })
         interactive.removeWindowOpenState(typedWindowId)
-        markWindowDirty(typedWindowId)
+        // One-shot: the rebuild re-derives the pose from the persisted node.
+        scene.markDirty(typedWindowId)
       } else {
         interactive.setWindowOpenState(typedWindowId, { [animation.field]: animation.to })
+        if (!appliedDirectly) scene.markDirty(typedWindowId)
       }
       emitter.emit('window:animation-completed', {
         windowId: typedWindowId as WindowNode['id'],

packages/viewer/src/systems/window/window-system.tsx

@@ -54,6 +54,11 @@ const MAX_WINDOW_REBUILDS_PER_FRAME = 16
 const WINDOW_PROGRESSIVE_DIRTY_THRESHOLD = MAX_WINDOW_REBUILDS_PER_FRAME
 const WINDOW_PROGRESSIVE_TIME_BUDGET_MS = 8
 
+// Transient rebuild requests from WindowAnimationSystem for windows whose type
+// has no direct pose path: drained every frame. Deliberately not dirtyNodes —
+// a running animation must not keep the dirty set from reaching zero.
+export const pendingWindowAnimationRebuilds = new Set<AnyNodeId>()
+
 export const WindowSystem = () => {
   const dirtyNodes = useScene((state) => state.dirtyNodes)
   const clearDirty = useScene((state) => state.clearDirty)
@@ -100,7 +105,7 @@ export const WindowSystem = () => {
   }, [sceneMaterials])
 
   useFrame(() => {
-    if (dirtyNodes.size === 0) return
+    if (dirtyNodes.size === 0 && pendingWindowAnimationRebuilds.size === 0) return
     baseMaterial = textures
       ? getBaseMaterial(shading)
       : createSurfaceRoleMaterial('joinery', colorPreset)
@@ -120,6 +125,12 @@ export const WindowSystem = () => {
       if (node?.type !== 'window') return
       dirtyWindowIds.push(id as AnyNodeId)
     })
+    if (pendingWindowAnimationRebuilds.size > 0) {
+      for (const id of pendingWindowAnimationRebuilds) {
+        if (nodes[id]?.type === 'window' && !dirtyWindowIds.includes(id)) dirtyWindowIds.push(id)
+      }
+      pendingWindowAnimationRebuilds.clear()
+    }
 
     const useProgressiveWindowRebuilds = dirtyWindowIds.length > WINDOW_PROGRESSIVE_DIRTY_THRESHOLD
     const frameStartedAt = performance.now()