From e66adf75cf1e97b5272cfd8ca8e3890450311651 Mon Sep 17 00:00:00 2001 From: Julien Brissonneau Date: Thu, 20 Aug 2026 14:19:34 -0400 Subject: [PATCH 1/2] fix(nodes): door/window MOVE drags ride the node's own wall and undo as one step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two night-6 QA defects in the opening move tools, both fallout around #689's hidden-wall pointer hold: 1. Wrong-wall capture: with every hidden wall a ray target, nearest-hit-wins let a hidden wall interposed between the camera and the dragged opening's own wall catch the wall:move stream — the drag rode a wall the user cannot see and the commit silently re-parented the opening onto it. New `shouldIgnoreWallEventForOpeningMove` gate (shared/opening-move-wall-gate): while MOVING an existing opening, a hidden wall may drive the drag only if it is the node's own wall (grab wall / current mid-drag host); visible walls always pass, so cross-wall re-parenting needs an explicit, visible target. Ignored events don't stop propagation, so the ray falls through to the own wall behind. PLACE / isNew duplicates keep the all-walls behavior. 2. Multi-entry undo per drag gesture: the tools paused history with a RAW `temporal.pause()`, invisible to the refcounted getSceneHistoryPauseDepth. zundo reads isTracking AFTER a write's subscribers run, so a cooperating system's balanced pauseSceneHistory/resumeSceneHistory pair (the space-detection sync, on any mid-drag reparent that touches wall children) zeroed the refcount and resumed tracking mid-gesture — the mid-drag writes became their own undo entries (transient states: commit fired at drag-arm, door hidden/reparented, orphan opening at the drop spot). New `beginOpeningMoveHistorySession` (shared/opening-move-history) holds the refcounted LEASE for the gesture and opens one deliberate tracking window (`commitStep`) for the drop write: mid-drag tracks nothing, drop records exactly one entry whose past state is the restored pre-drag baseline, cancel records nothing. Store-level tests pin the one-entry contract, the exact-baseline undo, lease composition, and the raw-pause leak this replaces. Co-Authored-By: Claude Fable 5 --- packages/nodes/src/door/move-tool.tsx | 109 ++++-- .../src/shared/opening-move-history.test.ts | 332 ++++++++++++++++++ .../nodes/src/shared/opening-move-history.ts | 64 ++++ .../src/shared/opening-move-wall-gate.test.ts | 139 ++++++++ .../src/shared/opening-move-wall-gate.ts | 62 ++++ packages/nodes/src/window/move-tool.tsx | 127 +++++-- 6 files changed, 764 insertions(+), 69 deletions(-) create mode 100644 packages/nodes/src/shared/opening-move-history.test.ts create mode 100644 packages/nodes/src/shared/opening-move-history.ts create mode 100644 packages/nodes/src/shared/opening-move-wall-gate.test.ts create mode 100644 packages/nodes/src/shared/opening-move-wall-gate.ts diff --git a/packages/nodes/src/door/move-tool.tsx b/packages/nodes/src/door/move-tool.tsx index db9b11ac5..ac6d4f343 100644 --- a/packages/nodes/src/door/move-tool.tsx +++ b/packages/nodes/src/door/move-tool.tsx @@ -37,6 +37,11 @@ import { clearOpeningGuides3D, publishOpeningGuidesForWallEvent, } from '../shared/opening-guides-runtime' +import { beginOpeningMoveHistorySession } from '../shared/opening-move-history' +import { + isWallMeshHidden, + shouldIgnoreWallEventForOpeningMove, +} from '../shared/opening-move-wall-gate' import { getRoofWallOpeningCursorPose, type RoofWallOpeningTarget, @@ -99,7 +104,13 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => }, []) useEffect(() => { - useScene.temporal.getState().pause() + // One undo entry per gesture: hold the REFCOUNTED history pause for the + // move's lifetime (a raw `temporal.pause()` is invisible to + // `getSceneHistoryPauseDepth()`, so a cooperating system's balanced + // pause/resume pair could zero the refcount mid-drag and resume tracking + // — every mid-drag write then became its own undo entry). The commit + // paths run their single tracked write through `history.commitStep`. + const history = beginOpeningMoveHistorySession() // This tool's whole cursor model is the wall surface (`wall:enter` / // `wall:move` / `wall:click`). Walls hidden by the wall-mode pass (X-ray // 'down' mode) are pointer-transparent for selection; hold their pointer @@ -279,6 +290,20 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => } } + // While MOVING an existing door, a HIDDEN wall may drive the drag only if + // it is the door's own wall (grab wall / current mid-drag host) — an + // interposed hidden wall between the camera and the door's wall must not + // capture the drag and silently re-parent the door on commit. Ignored + // events are NOT stopPropagation'd, so the ray falls through to the own + // wall behind. Fresh placements (`isNew`) keep the all-walls behavior. + const wallEventIgnored = (event: WallEvent) => + !isNew && + shouldIgnoreWallEventForOpeningMove({ + eventWallId: event.node.id, + eventWallHidden: isWallMeshHidden(event.node.id), + ownWallIds: [original.wallId, currentHostId], + }) + const resolveMoveTarget = (event: WallEvent) => { if (!isValidWallSideFace(event.normal)) return if (isCurvedWall(event.node)) { @@ -446,6 +471,10 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => } const onWallEnter = (event: WallEvent) => { + // Interposed hidden wall: ignore WITHOUT tearing down the current + // preview or stopping propagation — the own wall behind it (a later, + // farther intersection on this same ray) emits its own event. + if (wallEventIgnored(event)) return const target = resolveMoveTarget(event) if (!target) { onWallLeave() @@ -462,6 +491,8 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => } const onWallMove = (event: WallEvent) => { + // See onWallEnter — interposed hidden walls never own the move. + if (wallEventIgnored(event)) return if (!isValidWallSideFace(event.normal)) { onWallLeave() return @@ -499,8 +530,10 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => let placedId: string if (isNew) { + // Duplicate mode: delete the transient draft while history is still + // paused, then create the real node as the gesture's ONE tracked + // write — undo removes the new door entirely. useScene.getState().deleteNode(movingDoorNode.id) - useScene.temporal.getState().resume() const cloned = structuredClone(movingDoorNode) as any delete cloned.id @@ -518,9 +551,14 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => // must be visible regardless of the pre-commit free-follow state. visible: true, }) - useScene.getState().createNode(node, target.wallId as AnyNodeId) + history.commitStep(() => { + useScene.getState().createNode(node, target.wallId as AnyNodeId) + }) placedId = node.id } else { + // Move mode: restore the exact pre-drag state while history is still + // paused (the clean undo baseline), then apply the drop as the + // gesture's ONE tracked write — undo reverts to the original state. useScene.getState().updateNode(movingDoorNode.id, { position: original.position, rotation: original.rotation, @@ -532,17 +570,18 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => metadata: original.metadata, visible: original.visible, }) - useScene.temporal.getState().resume() - useScene.getState().updateNode(movingDoorNode.id, { - position: [target.clampedX, target.clampedY, 0], - rotation: [0, target.itemRotation, 0], - side: target.side, - parentId: target.wallId, - wallId: target.wallId, - roofSegmentId: undefined, - metadata: {}, - visible: true, + history.commitStep(() => { + useScene.getState().updateNode(movingDoorNode.id, { + position: [target.clampedX, target.clampedY, 0], + rotation: [0, target.itemRotation, 0], + side: target.side, + parentId: target.wallId, + wallId: target.wallId, + roofSegmentId: undefined, + metadata: {}, + visible: true, + }) }) if (original.parentId && original.parentId !== target.wallId) { @@ -553,7 +592,6 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => markHostDirty(target.wallId) useLiveTransforms.getState().clear(movingDoorNode.id) - useScene.temporal.getState().pause() triggerSFX('sfx:structure-build') hideCursor() @@ -563,6 +601,9 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => const onWallClick = (event: WallEvent) => { if (committed) return + // A click on an interposed hidden wall must not commit / re-parent; + // let it fall through to the own wall behind (see onWallEnter). + if (wallEventIgnored(event)) return if (!isValidWallSideFace(event.normal)) return if (isCurvedWall(event.node)) return if (event.node.parentId !== getLevelId()) return @@ -755,8 +796,9 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => let placedId: string if (isNew) { + // See commitToWall — delete the draft paused, create as the ONE + // tracked write. useScene.getState().deleteNode(movingDoorNode.id) - useScene.temporal.getState().resume() const cloned = structuredClone(movingDoorNode) as any delete cloned.id @@ -772,9 +814,13 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => parentId: segmentId, visible: true, }) - useScene.getState().createNode(node, segmentId as AnyNodeId) + history.commitStep(() => { + useScene.getState().createNode(node, segmentId as AnyNodeId) + }) placedId = node.id } else { + // See commitToWall — restore the pre-drag baseline paused, drop as + // the ONE tracked write. useScene.getState().updateNode(movingDoorNode.id, { position: original.position, rotation: original.rotation, @@ -786,18 +832,19 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => metadata: original.metadata, visible: original.visible, }) - useScene.temporal.getState().resume() - useScene.getState().updateNode(movingDoorNode.id, { - position: target.position, - rotation: [0, 0, 0], - side: 'front', - parentId: segmentId, - wallId: undefined, - roofSegmentId: segmentId, - roofFace: target.face.id, - metadata: {}, - visible: true, + history.commitStep(() => { + useScene.getState().updateNode(movingDoorNode.id, { + position: target.position, + rotation: [0, 0, 0], + side: 'front', + parentId: segmentId, + wallId: undefined, + roofSegmentId: segmentId, + roofFace: target.face.id, + metadata: {}, + visible: true, + }) }) if (original.parentId && original.parentId !== segmentId) { @@ -808,7 +855,6 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => markHostDirty(segmentId) useLiveTransforms.getState().clear(movingDoorNode.id) - useScene.temporal.getState().pause() triggerSFX('sfx:structure-build') hideCursor() @@ -846,7 +892,10 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => }) if (original.parentId) markHostDirty(original.parentId) } - useScene.temporal.getState().resume() + // The revert writes above ran under the gesture's history pause (never + // tracked); ending the session here keeps a cancelled move out of undo + // entirely. `end` is idempotent — the effect cleanup's end() is a no-op. + history.end() hideCursor() exitMoveMode() } @@ -1013,7 +1062,7 @@ const MoveDoorTool: React.FC<{ node: DoorNode }> = ({ node: movingDoorNode }) => useFacingPose.getState().clear() clearPlacementSurface() releaseHiddenWallHold() - useScene.temporal.getState().resume() + history.end() emitter.off('wall:enter', onWallEnter) emitter.off('wall:move', onWallMove) emitter.off('wall:click', onWallClick) diff --git a/packages/nodes/src/shared/opening-move-history.test.ts b/packages/nodes/src/shared/opening-move-history.test.ts new file mode 100644 index 000000000..bfd749bb2 --- /dev/null +++ b/packages/nodes/src/shared/opening-move-history.test.ts @@ -0,0 +1,332 @@ +import { afterEach, beforeEach, describe, expect, test } from 'bun:test' +import { + type AnyNode, + type AnyNodeId, + BuildingNode, + clearSceneHistory, + DoorNode, + getSceneHistoryPauseDepth, + LevelNode, + pauseSceneHistory, + resumeSceneHistory, + useScene, + WallNode, +} from '@pascal-app/core' +import { beginOpeningMoveHistorySession } from './opening-move-history' + +// `updateNodesAction` batches dirty-marking through requestAnimationFrame. +type RafFn = (cb: (time: number) => void) => number +;(globalThis as unknown as { requestAnimationFrame?: RafFn }).requestAnimationFrame ??= (cb) => { + cb(0) + return 0 +} +;(globalThis as unknown as { cancelAnimationFrame?: (id: number) => void }).cancelAnimationFrame ??= + () => {} + +// The night-6 door-drag undo defect, pinned at the store level. +// +// The door / window MOVE tools write the scene mid-drag (arm-time +// isTransient stamp, host-change reparents, floor free-follow) under a +// history pause, then commit the drop as one tracked write against a +// restored baseline. The old implementation paused with a RAW +// `useScene.temporal.getState().pause()` — invisible to the refcounted +// `getSceneHistoryPauseDepth()`. zundo evaluates `isTracking` AFTER a +// write's subscribers have run, so any cooperating system that takes a +// BALANCED `pauseSceneHistory`/`resumeSceneHistory` pair inside one of +// those mid-drag writes (the space-detection sync does, whenever a reparent +// touches a wall's `children`) zeroed the refcount, resumed tracking, and +// the mid-drag write that triggered it — plus every write after — became +// its own undo entry. QA saw a scene commit fire the moment the drag armed +// and a completed drag leave several undo states, none of them the baseline. +// +// `beginOpeningMoveHistorySession` holds the refcounted LEASE instead, so +// the depth stays ≥ 1 for the whole gesture: cooperating systems stand down +// (their gate sees the interaction) and no balanced pair can resume tracking +// mid-drag. `commitStep` opens the single deliberate tracking window. + +const BUILDING_ID = 'building_history' as AnyNodeId +const LEVEL_ID = 'level_history' as AnyNodeId +const WALL_A_ID = 'wall_history_own' as AnyNodeId +const WALL_B_ID = 'wall_history_other' as AnyNodeId +const DOOR_ID = 'door_history' as AnyNodeId + +function resetScene(): void { + const door = DoorNode.parse({ + id: DOOR_ID, + parentId: WALL_A_ID, + wallId: WALL_A_ID, + position: [1.5, 1.05, 0], + width: 0.9, + }) + const wallA = WallNode.parse({ + id: WALL_A_ID, + parentId: LEVEL_ID, + start: [0, 0], + end: [6, 0], + children: [DOOR_ID], + }) + const wallB = WallNode.parse({ + id: WALL_B_ID, + parentId: LEVEL_ID, + start: [0, -2.5], + end: [6, -2.5], + children: [], + }) + const level = LevelNode.parse({ + id: LEVEL_ID, + parentId: BUILDING_ID, + children: [WALL_A_ID, WALL_B_ID], + level: 0, + }) + const building = BuildingNode.parse({ + id: BUILDING_ID, + parentId: null, + children: [LEVEL_ID], + }) + useScene.setState({ + nodes: { + [BUILDING_ID]: building, + [LEVEL_ID]: level, + [WALL_A_ID]: wallA, + [WALL_B_ID]: wallB, + [DOOR_ID]: door, + }, + rootNodeIds: [BUILDING_ID], + dirtyNodes: new Set(), + collections: {}, + materials: {}, + readOnly: false, + } as never) + clearSceneHistory() +} + +function node(id: AnyNodeId): AnyNode { + const found = useScene.getState().nodes[id] + if (!found) throw new Error(`missing node ${id}`) + return found +} + +function pastLength(): number { + return useScene.temporal.getState().pastStates.length +} + +/** + * A stand-in for the space-detection sync (and any other cooperating + * system): stands down while an interaction holds the refcounted pause, + * otherwise brackets its reaction in a BALANCED pause/resume pair. With the + * old raw-pause tools this pair was the resume leak. + */ +function attachCooperatingSubscriber() { + let runs = 0 + let reentrant = false + const unsubscribe = useScene.subscribe(() => { + if (reentrant) return + if (getSceneHistoryPauseDepth() > 0) return + reentrant = true + try { + runs += 1 + pauseSceneHistory(useScene) + resumeSceneHistory(useScene) + } finally { + reentrant = false + } + }) + return { unsubscribe, ranTimes: () => runs } +} + +/** The MOVE tools' mid-drag write sequence: arm, free-follow, re-snap. */ +function writeMidDragSequence(): void { + const scene = useScene.getState() + // Drag arms: the tool stamps the node transient. + scene.updateNode(DOOR_ID, { metadata: { isTransient: true } }) + // Floor free-follow: reparent to the level, hidden (wall A's children change). + scene.updateNode(DOOR_ID, { + position: [2, 1.05, -1.2], + rotation: [0, 0, 0], + parentId: LEVEL_ID, + wallId: undefined, + visible: false, + }) + // Re-snap onto wall B (both walls' children change — the write that used + // to wake the space-detection pause/resume pair mid-drag). + scene.updateNode(DOOR_ID, { + position: [0.8, 1.05, 0], + rotation: [0, Math.PI, 0], + parentId: WALL_B_ID, + wallId: WALL_B_ID, + visible: false, + }) + // Slide along wall B. + scene.updateNode(DOOR_ID, { position: [2.4, 1.05, 0] }) +} + +/** The MOVE tools' commit: restore the baseline paused, drop as ONE tracked write. */ +function restoreBaselineThenCommit(session: ReturnType) { + const scene = useScene.getState() + scene.updateNode(DOOR_ID, { + position: [1.5, 1.05, 0], + rotation: [0, 0, 0], + parentId: WALL_A_ID, + wallId: WALL_A_ID, + metadata: {}, + visible: true, + }) + session.commitStep(() => { + scene.updateNode(DOOR_ID, { + position: [3.1, 1.05, 0], + rotation: [0, Math.PI, 0], + parentId: WALL_B_ID, + wallId: WALL_B_ID, + metadata: {}, + visible: true, + }) + }) +} + +describe('opening move history session', () => { + beforeEach(resetScene) + afterEach(() => { + clearSceneHistory() + }) + + test('a completed gesture is EXACTLY ONE undo entry; undo restores the pre-drag state', () => { + const cooperating = attachCooperatingSubscriber() + try { + const session = beginOpeningMoveHistorySession() + + writeMidDragSequence() + // Mid-drag: nothing tracked, tracking still off, interaction visible + // to cooperating systems (they stand down instead of leaking a resume). + expect(pastLength()).toBe(0) + expect(useScene.temporal.getState().isTracking).toBe(false) + expect(getSceneHistoryPauseDepth()).toBeGreaterThan(0) + expect(cooperating.ranTimes()).toBe(0) + + restoreBaselineThenCommit(session) + session.end() + + // Drop wrote exactly one entry; the session released its lease fully. + expect(pastLength()).toBe(1) + expect(getSceneHistoryPauseDepth()).toBe(0) + expect(useScene.temporal.getState().isTracking).toBe(true) + // The cooperating system got its normal look-in during the tracked drop. + expect(cooperating.ranTimes()).toBeGreaterThan(0) + + // The door committed to wall B... + expect(node(DOOR_ID).parentId).toBe(WALL_B_ID) + expect((node(WALL_B_ID) as { children: string[] }).children).toContain(DOOR_ID) + expect((node(WALL_A_ID) as { children: string[] }).children).not.toContain(DOOR_ID) + + // ...and ONE undo restores the exact pre-drag world: position, host, + // wall link, metadata, visibility, and both walls' children. + useScene.temporal.getState().undo() + const restored = node(DOOR_ID) as unknown as { + position: number[] + parentId: string + wallId?: string + metadata: unknown + visible?: boolean + } + expect(restored.position).toEqual([1.5, 1.05, 0]) + expect(restored.parentId).toBe(WALL_A_ID) + expect(restored.wallId).toBe(WALL_A_ID) + expect((restored.metadata ?? {}) as Record).not.toHaveProperty( + 'isTransient', + ) + expect(restored.visible).not.toBe(false) + expect((node(WALL_A_ID) as { children: string[] }).children).toContain(DOOR_ID) + expect((node(WALL_B_ID) as { children: string[] }).children).not.toContain(DOOR_ID) + expect(pastLength()).toBe(0) + + // Redo re-applies the drop — the gesture is one atomic step both ways. + useScene.temporal.getState().redo() + expect(node(DOOR_ID).parentId).toBe(WALL_B_ID) + expect(useScene.temporal.getState().futureStates.length).toBe(0) + } finally { + cooperating.unsubscribe() + } + }) + + test('a cancelled gesture leaves NO undo entry and the pre-drag state intact', () => { + const session = beginOpeningMoveHistorySession() + writeMidDragSequence() + + // The tools' cancel path: revert while paused, then end the session. + useScene.getState().updateNode(DOOR_ID, { + position: [1.5, 1.05, 0], + rotation: [0, 0, 0], + parentId: WALL_A_ID, + wallId: WALL_A_ID, + metadata: {}, + visible: true, + }) + session.end() + // Cancel (tool:cancel) and the effect cleanup BOTH end the session; + // the second end must be a no-op, not an underflow of someone else's pause. + session.end() + + expect(pastLength()).toBe(0) + expect(getSceneHistoryPauseDepth()).toBe(0) + expect(useScene.temporal.getState().isTracking).toBe(true) + expect(node(DOOR_ID).parentId).toBe(WALL_A_ID) + expect((node(WALL_A_ID) as { children: string[] }).children).toContain(DOOR_ID) + expect((node(WALL_B_ID) as { children: string[] }).children).not.toContain(DOOR_ID) + }) + + test('writes after commitStep (tool teardown) stay untracked until end()', () => { + const session = beginOpeningMoveHistorySession() + writeMidDragSequence() + restoreBaselineThenCommit(session) + expect(pastLength()).toBe(1) + + // Teardown-time write (e.g. a safety-net visibility restore) — the + // re-acquired lease keeps it out of history. + useScene.getState().updateNode(DOOR_ID, { visible: true }) + expect(pastLength()).toBe(1) + + session.end() + expect(getSceneHistoryPauseDepth()).toBe(0) + expect(pastLength()).toBe(1) + }) + + test('the session composes with an outer pause owner (never steals its pause)', () => { + pauseSceneHistory(useScene) + const session = beginOpeningMoveHistorySession() + writeMidDragSequence() + restoreBaselineThenCommit(session) + session.end() + + // The outer owner is still pausing: the commit write could not track and + // the depth still reflects the outer pause. + expect(pastLength()).toBe(0) + expect(getSceneHistoryPauseDepth()).toBe(1) + expect(useScene.temporal.getState().isTracking).toBe(false) + + resumeSceneHistory(useScene) + expect(getSceneHistoryPauseDepth()).toBe(0) + expect(useScene.temporal.getState().isTracking).toBe(true) + }) + + test('REGRESSION the raw temporal.pause() the session replaces leaked undo entries', () => { + const cooperating = attachCooperatingSubscriber() + try { + // The old tool pattern: a raw pause, invisible to the refcount. + useScene.temporal.getState().pause() + expect(getSceneHistoryPauseDepth()).toBe(0) + + writeMidDragSequence() + + // The cooperating subscriber saw depth 0, ran its balanced + // pause/resume pair, and RESUMED tracking out from under the raw + // pause — zundo reads isTracking after subscribers, so the mid-drag + // writes themselves were recorded as undo entries (transient states: + // door hidden / reparented mid-drag), none of them the baseline. + expect(cooperating.ranTimes()).toBeGreaterThan(0) + expect(useScene.temporal.getState().isTracking).toBe(true) + expect(pastLength()).toBeGreaterThan(1) + } finally { + cooperating.unsubscribe() + useScene.temporal.getState().resume() + } + }) +}) diff --git a/packages/nodes/src/shared/opening-move-history.ts b/packages/nodes/src/shared/opening-move-history.ts new file mode 100644 index 000000000..fe46b02e8 --- /dev/null +++ b/packages/nodes/src/shared/opening-move-history.ts @@ -0,0 +1,64 @@ +import { acquireSceneHistoryPause, useScene } from '@pascal-app/core' + +/** + * One-undo-entry-per-gesture history session for the door / window MOVE + * tools (the E5 drag-commit contract: mid-drag writes none, drop writes + * exactly one, undo restores the exact pre-drag state). + * + * The tools previously called `useScene.temporal.getState().pause()` / + * `.resume()` RAW. That pause is invisible to the refcounted + * `getSceneHistoryPauseDepth()` every cooperating system checks, which + * broke the gesture's atomicity two ways: + * + * 1. zundo reads `isTracking` AFTER the store's subscribers run for a + * write. A subscriber that takes a balanced + * `pauseSceneHistory`/`resumeSceneHistory` pair during a mid-drag + * write (the space-detection sync does exactly this when a reparent + * touches a wall's `children`) sees depth 0 → its resume re-enables + * tracking — and the mid-drag write that TRIGGERED it, plus every + * write after, lands in `pastStates`. That is night-6's door-drag + * undo defect: a scene commit fired the moment the drag armed, and a + * completed drag left multiple undo entries, none of them the + * baseline (door isTransient/invisible, parented to the level, an + * orphan opening at the drop spot...). + * 2. Systems that stand down during interactions gate on + * `getSceneHistoryPauseDepth() > 0`; a raw pause never registered, so + * they kept reconciling against half-written mid-drag states. + * + * This session holds a refcounted LEASE (`acquireSceneHistoryPause`) + * instead. While it is held the depth is ≥ 1, so cooperating systems both + * see the interaction and — crucially — can no longer zero the refcount + * and resume tracking out from under the gesture. `commitStep` opens the + * one deliberate tracking window for the drop write; `end` releases the + * lease (idempotent, safe to call from both cancel and effect cleanup). + */ +export type OpeningMoveHistorySession = { + /** + * Run the gesture's single committing write with history tracking live: + * releases the lease for exactly this call, then re-acquires it so any + * teardown writes that follow (tool unmount, selection churn) stay out + * of history. The caller restores the node to its exact pre-drag state + * (still paused) right BEFORE this, so the one entry zundo records has + * the true baseline as its past state. + */ + commitStep(write: () => T): T + /** Release the gesture's history pause. Idempotent. */ + end(): void +} + +export const beginOpeningMoveHistorySession = (): OpeningMoveHistorySession => { + let release = acquireSceneHistoryPause(useScene) + return { + commitStep(write) { + release() + try { + return write() + } finally { + release = acquireSceneHistoryPause(useScene) + } + }, + end() { + release() + }, + } +} diff --git a/packages/nodes/src/shared/opening-move-wall-gate.test.ts b/packages/nodes/src/shared/opening-move-wall-gate.test.ts new file mode 100644 index 000000000..9db8ecae5 --- /dev/null +++ b/packages/nodes/src/shared/opening-move-wall-gate.test.ts @@ -0,0 +1,139 @@ +import { afterEach, describe, expect, test } from 'bun:test' +import { sceneRegistry } from '@pascal-app/core' +import { Group } from 'three' +import { isWallMeshHidden, shouldIgnoreWallEventForOpeningMove } from './opening-move-wall-gate' + +// Semantics pinned here (the door / window MOVE tools evaluate this predicate +// on every wall:enter / wall:move / wall:click before resolving a target): +// - #689 / night-6: while an opening tool is active, hidden walls stay ray +// targets (the pointer hold) — but nearest-hit-wins let a hidden wall +// INTERPOSED between the camera and the dragged opening's own wall capture +// the drag, and the commit silently re-parented the opening onto a wall the +// user cannot see (QA: window wall_pgmay5kic2q0umkz → wall_n2u7vn4nfimt2bom). +// - The MOVE tools therefore ignore hidden walls that are not the node's own +// (grab wall or current mid-drag host). Ignored events do not stop +// propagation, so the ray falls through to the own wall behind. +// - VISIBLE walls always pass: cross-wall re-parenting stays possible, but +// only onto an explicit target the user can see. +// - PLACE (fresh openings, incl. `metadata.isNew` duplicates) skips the gate: +// placing onto any wall — hidden ones included — is the X-ray experience. + +describe('shouldIgnoreWallEventForOpeningMove', () => { + const OWN_WALL = 'wall_own' + const OTHER_WALL = 'wall_interposed' + + test('interposed HIDDEN wall: ignored (the wrong-wall capture fix)', () => { + expect( + shouldIgnoreWallEventForOpeningMove({ + eventWallId: OTHER_WALL, + eventWallHidden: true, + ownWallIds: [OWN_WALL, OWN_WALL], + }), + ).toBe(true) + }) + + test("the node's OWN hidden wall: never ignored (X-ray drags keep sliding, #689)", () => { + expect( + shouldIgnoreWallEventForOpeningMove({ + eventWallId: OWN_WALL, + eventWallHidden: true, + ownWallIds: [OWN_WALL, null], + }), + ).toBe(false) + }) + + test('current mid-drag host counts as an own wall even when hidden', () => { + expect( + shouldIgnoreWallEventForOpeningMove({ + eventWallId: OTHER_WALL, + eventWallHidden: true, + // Grabbed from OWN_WALL, legitimately re-parented to OTHER_WALL while + // it was visible; it may keep the drag if the camera later hides it. + ownWallIds: [OWN_WALL, OTHER_WALL], + }), + ).toBe(false) + }) + + test('VISIBLE walls always pass — explicit cross-wall re-parenting stays possible', () => { + for (const ownWallIds of [ + [OWN_WALL, OWN_WALL], + [OWN_WALL, null], + [undefined, null], + ]) { + expect( + shouldIgnoreWallEventForOpeningMove({ + eventWallId: OTHER_WALL, + eventWallHidden: false, + ownWallIds, + }), + ).toBe(false) + } + }) + + test('free-follow host (a level id) and empty own ids never match a wall event', () => { + // Mid-drag over open floor the opening parents to the LEVEL; the level id + // must not accidentally whitelist a hidden wall. + expect( + shouldIgnoreWallEventForOpeningMove({ + eventWallId: OTHER_WALL, + eventWallHidden: true, + ownWallIds: [OWN_WALL, 'level_ground'], + }), + ).toBe(true) + // Roof-hosted openings have no wallId at grab; every hidden wall is then + // a non-own wall until an explicit visible re-parent. + expect( + shouldIgnoreWallEventForOpeningMove({ + eventWallId: OTHER_WALL, + eventWallHidden: true, + ownWallIds: [undefined, 'roofseg_a'], + }), + ).toBe(true) + }) +}) + +describe('isWallMeshHidden', () => { + afterEach(() => { + sceneRegistry.nodes.delete('wall_gate_test') + }) + + test('reads the WallCutout wallHidden stamp off the registered mesh', () => { + const mesh = new Group() + sceneRegistry.nodes.set('wall_gate_test', mesh) + + expect(isWallMeshHidden('wall_gate_test')).toBe(false) + + mesh.userData.wallHidden = true + expect(isWallMeshHidden('wall_gate_test')).toBe(true) + + mesh.userData.wallHidden = false + expect(isWallMeshHidden('wall_gate_test')).toBe(false) + }) + + test('unregistered walls count as visible (nothing behind to fall through to)', () => { + expect(isWallMeshHidden('wall_never_registered')).toBe(false) + }) + + test('composes with the pure gate the way the move tools call it', () => { + const mesh = new Group() + mesh.userData.wallHidden = true + sceneRegistry.nodes.set('wall_gate_test', mesh) + + const ignored = (eventWallId: string) => + shouldIgnoreWallEventForOpeningMove({ + eventWallId, + eventWallHidden: isWallMeshHidden(eventWallId), + ownWallIds: ['wall_own', null], + }) + + // Hidden + not own → ignored; the same wall as own → allowed. + expect(ignored('wall_gate_test')).toBe(true) + expect( + shouldIgnoreWallEventForOpeningMove({ + eventWallId: 'wall_gate_test', + eventWallHidden: isWallMeshHidden('wall_gate_test'), + ownWallIds: ['wall_gate_test', null], + }), + ).toBe(false) + }) +}) diff --git a/packages/nodes/src/shared/opening-move-wall-gate.ts b/packages/nodes/src/shared/opening-move-wall-gate.ts new file mode 100644 index 000000000..a198e3f47 --- /dev/null +++ b/packages/nodes/src/shared/opening-move-wall-gate.ts @@ -0,0 +1,62 @@ +import { type AnyNodeId, sceneRegistry } from '@pascal-app/core' + +/** + * Hidden-wall gate for the door / window MOVE tools. + * + * #689's hidden-wall pointer hold keeps EVERY hidden wall a ray target while + * an opening tool is active — that fixed the drag detaching into the floor + * free-follow (red world-axis ghost) when the node's own wall is hidden in + * X-ray. But nearest-hit-wins over-corrected the MOVE tools: a hidden wall + * INTERPOSED between the camera and the dragged opening's own wall caught + * the `wall:move` stream, so the drag silently rode a wall the user cannot + * see and the commit RE-PARENTED the opening onto it (night-6 QA: a window + * moved along its z=0 wall landed on an invisible wall at z=-2.5). + * + * Rule — while MOVING an existing opening, a wall event may drive the drag + * only when: + * - the event's wall is one of the node's OWN walls (the wall it was + * grabbed from, or the host it legitimately re-parented to mid-drag), + * hidden or not — an X-ray drag along its own hidden wall keeps working + * exactly as #689 intended; or + * - the event's wall is VISIBLE — cross-wall re-parenting stays possible, + * but only onto a target the user can actually see. + * + * Ignored events must NOT stop propagation: R3F then continues down the + * intersection list, so the ray falls through the interposed hidden wall to + * the node's own wall behind it and the drag keeps riding the wall the user + * is reasoning about. If the ray misses the own wall entirely the existing + * off-wall handling (floor free-follow) takes over unchanged. + * + * PLACE (fresh door/window, incl. `metadata.isNew` duplicates) keeps the + * all-walls behavior — placing onto any wall, hidden ones included, is the + * intended X-ray experience; the tools skip this gate for those. + * + * Pure so the truth table is testable without an R3F rig (mirrors + * `wallPointerEventsSuppressed`); the tools supply live values per event. + */ +export const shouldIgnoreWallEventForOpeningMove = ({ + eventWallId, + eventWallHidden, + ownWallIds, +}: { + /** The wall that emitted the `wall:enter` / `wall:move` / `wall:click`. */ + eventWallId: string + /** Live hide state of that wall (the wall-mode pass, see `isWallMeshHidden`). */ + eventWallHidden: boolean + /** + * Walls the moving node may ride even while hidden: the wall it was + * grabbed from and its current mid-drag host. Non-wall entries (a level id + * during free-follow, a roof segment, `null`) simply never match. + */ + ownWallIds: ReadonlyArray +}): boolean => eventWallHidden && !ownWallIds.includes(eventWallId) + +/** + * Live hide state of a wall's registered mesh. `WallCutout` stamps + * `userData.wallHidden` on the wall's scene-registry mesh every pass (X-ray + * 'down' mode, cutaway-hidden faces, auto-mode interior partitions); the + * wall renderer's pointer gate reads the same stamp. Unregistered walls + * (not mounted yet) count as visible — there is nothing to fall through to. + */ +export const isWallMeshHidden = (wallId: string): boolean => + sceneRegistry.nodes.get(wallId as AnyNodeId)?.userData?.wallHidden === true diff --git a/packages/nodes/src/window/move-tool.tsx b/packages/nodes/src/window/move-tool.tsx index c0ba4f278..432510c2d 100644 --- a/packages/nodes/src/window/move-tool.tsx +++ b/packages/nodes/src/window/move-tool.tsx @@ -39,6 +39,11 @@ import { publishOpeningGuidesForWallEvent, resolveSillSnap, } from '../shared/opening-guides-runtime' +import { beginOpeningMoveHistorySession } from '../shared/opening-move-history' +import { + isWallMeshHidden, + shouldIgnoreWallEventForOpeningMove, +} from '../shared/opening-move-wall-gate' import { getRoofWallOpeningCursorPose, type RoofWallOpeningTarget, @@ -69,13 +74,15 @@ const edgeMaterial = new LineBasicNodeMaterial({ * Move/duplicate tool for WindowNodes — wall-only, same guardrails as WindowTool. * * Move mode (metadata.isNew falsy): - * Adopts the existing window, pauses temporal. On commit: restores original state - * (clean undo baseline) then resumes + updateNode (undo reverts to original position). - * On cancel: restores original state. + * Adopts the existing window and holds a refcounted history pause for the + * gesture. On commit: restores original state (clean undo baseline) then runs + * updateNode as the gesture's single tracked write (undo reverts to the + * original position). On cancel: restores original state, never tracked. * * Duplicate mode (metadata.isNew = true): - * The node is a freshly created transient copy. On commit: deletes transient + resumes - * + createNode (undo removes the new window entirely). On cancel: deletes the node. + * The node is a freshly created transient copy. On commit: deletes the + * transient paused + createNode as the single tracked write (undo removes the + * new window entirely). On cancel: deletes the node. */ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode }) => { const cursorGroupRef = useRef(null!) @@ -117,7 +124,13 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode }, []) useEffect(() => { - useScene.temporal.getState().pause() + // One undo entry per gesture: hold the REFCOUNTED history pause for the + // move's lifetime (a raw `temporal.pause()` is invisible to + // `getSceneHistoryPauseDepth()`, so a cooperating system's balanced + // pause/resume pair could zero the refcount mid-drag and resume tracking + // — every mid-drag write then became its own undo entry). The commit + // paths run their single tracked write through `history.commitStep`. + const history = beginOpeningMoveHistorySession() // This tool's whole cursor model is the wall surface (`wall:enter` / // `wall:move` / `wall:click`). Walls hidden by the wall-mode pass (X-ray // 'down' mode) are pointer-transparent for selection; hold their pointer @@ -302,6 +315,22 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode edgeMaterial.color.setHex(valid ? 0x22_c5_5e : 0xef_44_44) } + // While MOVING an existing window, a HIDDEN wall may drive the drag only + // if it is the window's own wall (grab wall / current mid-drag host) — an + // interposed hidden wall between the camera and the window's wall must + // not capture the drag and silently re-parent the window on commit + // (night-6 QA: an X-ray drag rode an invisible wall at z=-2.5 instead of + // the window's own wall at z=0). Ignored events are NOT + // stopPropagation'd, so the ray falls through to the own wall behind. + // Fresh placements (`isNew`) keep the all-walls behavior. + const wallEventIgnored = (event: WallEvent) => + !isNew && + shouldIgnoreWallEventForOpeningMove({ + eventWallId: event.node.id, + eventWallHidden: isWallMeshHidden(event.node.id), + ownWallIds: [original.wallId, currentHostId], + }) + const resolveMoveTarget = (event: WallEvent) => { if (!isValidWallSideFace(event.normal)) return if (isCurvedWall(event.node)) { @@ -485,6 +514,10 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode } const onWallEnter = (event: WallEvent) => { + // Interposed hidden wall: ignore WITHOUT tearing down the current + // preview or stopping propagation — the own wall behind it (a later, + // farther intersection on this same ray) emits its own event. + if (wallEventIgnored(event)) return const target = resolveMoveTarget(event) if (!target) { onWallLeave() @@ -501,6 +534,8 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode } const onWallMove = (event: WallEvent) => { + // See onWallEnter — interposed hidden walls never own the move. + if (wallEventIgnored(event)) return if (!isValidWallSideFace(event.normal)) { onWallLeave() return @@ -539,10 +574,10 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode let placedId: string if (isNew) { - // Duplicate mode: delete transient + resume + createNode - // Undo will remove the newly created node entirely + // Duplicate mode: delete the transient draft while history is still + // paused, then create the real node as the gesture's ONE tracked + // write — undo removes the new window entirely. useScene.getState().deleteNode(movingWindowNode.id) - useScene.temporal.getState().resume() const cloned = structuredClone(movingWindowNode) as any delete cloned.id @@ -560,11 +595,14 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode // Hidden during free-follow; the committed window must be visible. visible: true, }) - useScene.getState().createNode(node, target.wallId as AnyNodeId) + history.commitStep(() => { + useScene.getState().createNode(node, target.wallId as AnyNodeId) + }) placedId = node.id } else { - // Move mode: restore original (clean baseline) + resume + updateNode - // Undo will revert to the original position + // Move mode: restore the exact pre-drag state while history is still + // paused (the clean undo baseline), then apply the drop as the + // gesture's ONE tracked write — undo reverts to the original state. useScene.getState().updateNode(movingWindowNode.id, { position: original.position, rotation: original.rotation, @@ -576,17 +614,18 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode metadata: original.metadata, visible: original.visible, }) - useScene.temporal.getState().resume() - useScene.getState().updateNode(movingWindowNode.id, { - position: [target.clampedX, target.clampedY, 0], - rotation: [0, target.itemRotation, 0], - side: target.side, - parentId: target.wallId, - wallId: target.wallId, - roofSegmentId: undefined, - metadata: {}, - visible: true, + history.commitStep(() => { + useScene.getState().updateNode(movingWindowNode.id, { + position: [target.clampedX, target.clampedY, 0], + rotation: [0, target.itemRotation, 0], + side: target.side, + parentId: target.wallId, + wallId: target.wallId, + roofSegmentId: undefined, + metadata: {}, + visible: true, + }) }) if (original.parentId && original.parentId !== target.wallId) { @@ -597,7 +636,6 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode markHostDirty(target.wallId) useLiveTransforms.getState().clear(movingWindowNode.id) - useScene.temporal.getState().pause() triggerSFX('sfx:structure-build') hideCursor() @@ -607,6 +645,9 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode const onWallClick = (event: WallEvent) => { if (committed) return + // A click on an interposed hidden wall must not commit / re-parent; + // let it fall through to the own wall behind (see onWallEnter). + if (wallEventIgnored(event)) return if (!isValidWallSideFace(event.normal)) return if (isCurvedWall(event.node)) return // Only interact with walls on the current level @@ -796,8 +837,9 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode let placedId: string if (isNew) { + // See commitToWall — delete the draft paused, create as the ONE + // tracked write. useScene.getState().deleteNode(movingWindowNode.id) - useScene.temporal.getState().resume() const cloned = structuredClone(movingWindowNode) as any delete cloned.id @@ -814,9 +856,13 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode parentId: segmentId, visible: true, }) - useScene.getState().createNode(node, segmentId as AnyNodeId) + history.commitStep(() => { + useScene.getState().createNode(node, segmentId as AnyNodeId) + }) placedId = node.id } else { + // See commitToWall — restore the pre-drag baseline paused, drop as + // the ONE tracked write. useScene.getState().updateNode(movingWindowNode.id, { position: original.position, rotation: original.rotation, @@ -828,18 +874,19 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode metadata: original.metadata, visible: original.visible, }) - useScene.temporal.getState().resume() - useScene.getState().updateNode(movingWindowNode.id, { - position: target.position, - rotation: [0, 0, 0], - side: 'front', - parentId: segmentId, - wallId: undefined, - roofSegmentId: segmentId, - roofFace: target.face.id, - metadata: {}, - visible: true, + history.commitStep(() => { + useScene.getState().updateNode(movingWindowNode.id, { + position: target.position, + rotation: [0, 0, 0], + side: 'front', + parentId: segmentId, + wallId: undefined, + roofSegmentId: segmentId, + roofFace: target.face.id, + metadata: {}, + visible: true, + }) }) if (original.parentId && original.parentId !== segmentId) { @@ -850,7 +897,6 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode markHostDirty(segmentId) useLiveTransforms.getState().clear(movingWindowNode.id) - useScene.temporal.getState().pause() triggerSFX('sfx:structure-build') hideCursor() @@ -888,7 +934,10 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode }) if (original.parentId) markHostDirty(original.parentId) } - useScene.temporal.getState().resume() + // The revert writes above ran under the gesture's history pause (never + // tracked); ending the session here keeps a cancelled move out of undo + // entirely. `end` is idempotent — the effect cleanup's end() is a no-op. + history.end() hideCursor() exitMoveMode() } @@ -1048,7 +1097,7 @@ const MoveWindowTool: React.FC<{ node: WindowNode }> = ({ node: movingWindowNode useFacingPose.getState().clear() clearPlacementSurface() releaseHiddenWallHold() - useScene.temporal.getState().resume() + history.end() emitter.off('wall:enter', onWallEnter) emitter.off('wall:move', onWallMove) emitter.off('wall:click', onWallClick) From f028cc251f8db42e6e2c9a45596386111df610a3 Mon Sep 17 00:00:00 2001 From: Julien Brissonneau Date: Thu, 20 Aug 2026 14:20:29 -0400 Subject: [PATCH 2/2] style(nodes): biome format for the opening-move-history test Co-Authored-By: Claude Fable 5 --- packages/nodes/src/shared/opening-move-history.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/nodes/src/shared/opening-move-history.test.ts b/packages/nodes/src/shared/opening-move-history.test.ts index bfd749bb2..9fb9e8302 100644 --- a/packages/nodes/src/shared/opening-move-history.test.ts +++ b/packages/nodes/src/shared/opening-move-history.test.ts @@ -230,9 +230,7 @@ describe('opening move history session', () => { expect(restored.position).toEqual([1.5, 1.05, 0]) expect(restored.parentId).toBe(WALL_A_ID) expect(restored.wallId).toBe(WALL_A_ID) - expect((restored.metadata ?? {}) as Record).not.toHaveProperty( - 'isTransient', - ) + expect((restored.metadata ?? {}) as Record).not.toHaveProperty('isTransient') expect(restored.visible).not.toBe(false) expect((node(WALL_A_ID) as { children: string[] }).children).toContain(DOOR_ID) expect((node(WALL_B_ID) as { children: string[] }).children).not.toContain(DOOR_ID)