From c2c2f66427680725c91313a873553186e2041180 Mon Sep 17 00:00:00 2001 From: Wassim SAMAD Date: Mon, 18 May 2026 08:12:07 -0400 Subject: [PATCH] Phase 5 Stage D: fix bend history + move-tool loop + grid-snap sfx MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three user-reported regressions, all in the Stage D move/curve ports: 1. **Fence/wall bend cancelled the fence creation on Ctrl-Z.** The action.commit's `return false` shortcut on "no offset change" bypassed the dance entirely — pastStates wasn't touched, so the next Ctrl-Z fell through to whatever preceded activation (typically the create step). Fix: always run the dance, even on no-op commits. First Ctrl-Z then absorbs a silent no-op entry, subsequent presses roll back real prior actions. Same fix applied to fence move-endpoint, fence move, slab move, ceiling move. 2. **Slab/ceiling move 'maximum update depth exceeded' loop.** The `useScene` selector in `SlabMoveTool` returned a freshly-allocated `[sx, sz]` tuple on every call. Zustand's `Object.is` equality failed each comparison → re-subscribe → re-render → loop. Fix: subscribe to the stable live-node reference and derive the center via `useMemo`. Same recipe for fence/ceiling move-tools, including memoizing the `originalCenter` fallback that was getting a new array per render. 3. **No grid-snap sfx during move drag.** Action `preview` now tracks the last snapped pointer on a mutable `lastSnapped` ctx field and emits `sfx:grid-snap` when it changes between ticks. Matches the legacy MoveFenceTool's per-tick sound. Locking test for foot-gun 1 lives in `packages/core/src/services/single-undo-dance.test.ts`. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/nodes/src/ceiling/actions/move.ts | 21 +++++++-- packages/nodes/src/ceiling/move-tool.tsx | 6 ++- packages/nodes/src/fence/actions/curve.ts | 20 ++++---- .../nodes/src/fence/actions/move-endpoint.ts | 11 +++-- packages/nodes/src/fence/actions/move.ts | 36 ++++++++++----- packages/nodes/src/fence/move-tool.tsx | 36 +++++++++------ packages/nodes/src/slab/actions/move.ts | 24 +++++++--- packages/nodes/src/slab/move-tool.tsx | 46 +++++++++++-------- packages/nodes/src/wall/actions/curve.ts | 4 +- 9 files changed, 134 insertions(+), 70 deletions(-) diff --git a/packages/nodes/src/ceiling/actions/move.ts b/packages/nodes/src/ceiling/actions/move.ts index 14baedfb..9960f4be 100644 --- a/packages/nodes/src/ceiling/actions/move.ts +++ b/packages/nodes/src/ceiling/actions/move.ts @@ -1,4 +1,5 @@ import type { AnyNode, AnyNodeId, CeilingNode, DragAction } from '@pascal-app/core' +import { triggerSFX } from '@pascal-app/editor' /** * Phase 5 Stage D — whole-ceiling move drag affordance. @@ -6,7 +7,8 @@ import type { AnyNode, AnyNodeId, CeilingNode, DragAction } from '@pascal-app/co * Mirrors `slab/actions/move.ts` shape but ceiling snaps purely to a * 0.5m grid (no wall/fence corner snap — ceilings are typically * placed independent of the floor layout). Drag anchor is latched on - * the first preview tick so the ceiling doesn't jump. + * the first preview tick so the ceiling doesn't jump. Emits a grid- + * snap sfx when the snapped position changes between ticks. * * Single-undo dance on commit, same recipe as slab/fence. */ @@ -17,6 +19,10 @@ function snap(value: number): number { return Math.round(value / GRID_STEP) * GRID_STEP } +function sameSnap(a: [number, number] | null, b: [number, number]): boolean { + return a !== null && a[0] === b[0] && a[1] === b[1] +} + function translatePolygon( polygon: Array<[number, number]>, deltaX: number, @@ -30,6 +36,7 @@ export type MoveCeilingCtx = { originalPolygon: Array<[number, number]> originalHoles: Array> dragAnchor: [number, number] | null + lastSnapped: [number, number] | null } export type MoveCeilingDraft = { @@ -50,13 +57,19 @@ export const moveCeilingDragAction: DragAction h.map(([x, z]) => [x, z] as [number, number]), ), dragAnchor: null, + lastSnapped: null, } }, preview: (ctx, point, _modifiers) => { const sx = snap(point[0]) const sz = snap(point[1]) - if (!ctx.dragAnchor) ctx.dragAnchor = [sx, sz] + const snapped: [number, number] = [sx, sz] + if (!sameSnap(ctx.lastSnapped, snapped)) { + if (ctx.lastSnapped !== null) triggerSFX('sfx:grid-snap') + ctx.lastSnapped = snapped + } + if (!ctx.dragAnchor) ctx.dragAnchor = snapped const deltaX = sx - ctx.dragAnchor[0] const deltaZ = sz - ctx.dragAnchor[1] return { @@ -76,7 +89,9 @@ export const moveCeilingDragAction: DragAction }, commit: (draft, ctx, scene) => { - if (draft.deltaX === 0 && draft.deltaZ === 0) return false + // Always push — see fence/actions/curve.ts. No-movement still + // records a pastState entry so Ctrl-Z doesn't fall through to the + // ceiling-create step. scene.restoreAll() scene.resumeHistory() scene.update(ctx.ceilingId, { diff --git a/packages/nodes/src/ceiling/move-tool.tsx b/packages/nodes/src/ceiling/move-tool.tsx index 61a5b90a..8e8dd80d 100644 --- a/packages/nodes/src/ceiling/move-tool.tsx +++ b/packages/nodes/src/ceiling/move-tool.tsx @@ -22,7 +22,11 @@ export const CeilingMoveTool: React.FC<{ node: CeilingNode }> = ({ node }) => { const live = useScene((s) => s.nodes[ceilingId]) const liveCeiling = live?.type === 'ceiling' ? (live as CeilingNode) : node const polygon = liveCeiling.polygon - const holes = liveCeiling.holes ?? [] + // `?? []` would return a NEW empty array per render, busting downstream + // useMemo deps and (under StrictMode) potentially triggering the + // "getSnapshot result not cached" warning. Memoize against the source. + const EMPTY_HOLES = useMemo(() => [] as Array>, []) + const holes = liveCeiling.holes ?? EMPTY_HOLES const center: [number, number] = useMemo(() => { if (polygon.length === 0) return [0, 0] diff --git a/packages/nodes/src/fence/actions/curve.ts b/packages/nodes/src/fence/actions/curve.ts index cbda4265..eb5e7bf5 100644 --- a/packages/nodes/src/fence/actions/curve.ts +++ b/packages/nodes/src/fence/actions/curve.ts @@ -88,15 +88,17 @@ export const curveFenceDragAction: DragAction = }, commit: (draft, ctx, scene) => { - // Reject when the offset didn't actually change — createDragSession - // will fall through to cancel + scene.restoreAll() (no zundo entry). - if (draft.curveOffset === ctx.originalCurveOffset) return false - - // Single-undo dance: revert via the snapshot (paused history → no - // zundo record), resume history, then re-apply the final draft so - // zundo captures the whole drag as one undo step. Without this the - // pause window's mutations never reach pastStates and Ctrl-Z jumps - // past the drag back to the state before activation. + // Single-undo dance — ALWAYS push a pastState entry, even when the + // offset didn't actually change. The "no-op" case (small drag that + // `normalizeWallCurveOffset` snaps back to 0) used to return false + // here, but that bypassed pastStates entirely; the next Ctrl-Z then + // fell through to whatever was on the stack before activation + // (typically the fence creation), making it look like the bend + // cancelled the create. + // + // Pushing on every commit means a no-op bend's first Ctrl-Z absorbs + // a silent entry (no visible change), then subsequent Ctrl-Z's roll + // back the real prior actions. Matches typical editor behavior. scene.restoreAll() scene.resumeHistory() scene.update(ctx.nodeId, { curveOffset: draft.curveOffset } as Partial) diff --git a/packages/nodes/src/fence/actions/move-endpoint.ts b/packages/nodes/src/fence/actions/move-endpoint.ts index 5bf6bfbf..e79a2738 100644 --- a/packages/nodes/src/fence/actions/move-endpoint.ts +++ b/packages/nodes/src/fence/actions/move-endpoint.ts @@ -183,11 +183,12 @@ export const moveFenceEndpointDragAction: DragAction { - // Reject when the drag didn't move OR the resulting fence would - // be shorter than the minimum length. createDragSession.cancel() - // restores originals via the snapshot. - const hasChanged = !samePoint(draft.movingPoint, ctx.originalMovingPoint) - if (!hasChanged) return false + // Min-length rejection still matters — too-short fence is invalid + // and should bounce back via the cancel path (snapshot restore). + // But the "no-change" rejection is removed: see + // fence/actions/curve.ts for the rationale (no-op drag must still + // push a pastState entry to avoid Ctrl-Z cancelling the fence + // creation that preceded the activation). if (!isWallLongEnough(draft.start, draft.end)) return false // Single-undo dance: revert to originals (paused history → no diff --git a/packages/nodes/src/fence/actions/move.ts b/packages/nodes/src/fence/actions/move.ts index d9f7616e..78d61cd8 100644 --- a/packages/nodes/src/fence/actions/move.ts +++ b/packages/nodes/src/fence/actions/move.ts @@ -9,9 +9,13 @@ import { useScene, type WallNode, } from '@pascal-app/core' -import { type FencePlanPoint, snapFenceDraftPoint } from '@pascal-app/editor' +import { type FencePlanPoint, snapFenceDraftPoint, triggerSFX } from '@pascal-app/editor' import type * as THREE from 'three' +function sameSnap(a: FencePlanPoint | null, b: FencePlanPoint): boolean { + return a !== null && a[0] === b[0] && a[1] === b[1] +} + /** * Phase 5 Stage D — whole-fence move drag affordance. * @@ -137,6 +141,10 @@ export type MoveFenceCtx = { // Mutable: latched on the first preview call to the snapped pointer // position. Subsequent previews compute delta = pointer - dragAnchor. dragAnchor: FencePlanPoint | null + // Mutable: tracks the last snapped pointer so preview can emit a + // grid-snap sfx when the snapped value changes. Matches the legacy + // MoveFenceTool's per-tick sound. + lastSnapped: FencePlanPoint | null } export type MoveFenceDraft = { @@ -180,6 +188,7 @@ export const moveFenceDragAction: DragAction = { levelWalls, levelFences, dragAnchor: null, + lastSnapped: null, } }, @@ -190,6 +199,12 @@ export const moveFenceDragAction: DragAction = { fences: ctx.levelFences, ignoreFenceIds: [ctx.fenceId as string], }) + // Emit grid-snap sfx when the snapped position changes between + // ticks — matches the legacy MoveFenceTool's user feedback. + if (!sameSnap(ctx.lastSnapped, snapped)) { + if (ctx.lastSnapped !== null) triggerSFX('sfx:grid-snap') + ctx.lastSnapped = snapped + } // Latch the anchor on the first preview tick — matches legacy // "drag is delta from first move" semantics so the fence doesn't // jump to wherever the activation click landed. @@ -229,16 +244,15 @@ export const moveFenceDragAction: DragAction = { }, commit: (draft, ctx, scene) => { - // Reject when nothing moved — falls through to action.cancel which - // clears the live-drag visual state. - if (draft.deltaX === 0 && draft.deltaZ === 0) return false - - // Single-undo dance — paused history during drag means nothing - // was recorded. We didn't scene.update during apply either (live- - // drag exception), so the snapshot is empty and restoreAll is a - // no-op. Resume history, then write the final draft. Zundo - // captures one diff: original → final. - scene.restoreAll() // no-op (no scene updates during apply) + // Always push a pastState entry — see fence/actions/curve.ts. The + // no-movement case would otherwise let Ctrl-Z cancel the fence + // creation that preceded the move. + // + // Single-undo dance: snapshot is empty (live-drag exception, no + // scene.update during apply), so restoreAll is a no-op. Resume, + // then write the final draft so zundo records original → final + // as one diff. + scene.restoreAll() scene.resumeHistory() scene.update(ctx.fenceId, { start: draft.start, diff --git a/packages/nodes/src/fence/move-tool.tsx b/packages/nodes/src/fence/move-tool.tsx index c307216a..22e2ea78 100644 --- a/packages/nodes/src/fence/move-tool.tsx +++ b/packages/nodes/src/fence/move-tool.tsx @@ -3,6 +3,7 @@ import { type FenceNode, useLiveTransforms } from '@pascal-app/core' import { CursorSphere, triggerSFX, useDragAction, useEditor } from '@pascal-app/editor' import { useViewer } from '@pascal-app/viewer' +import { useMemo } from 'react' import { moveFenceDragAction } from './actions/move' /** @@ -10,8 +11,13 @@ import { moveFenceDragAction } from './actions/move' * * Replaces the legacy `MoveFenceTool` (302 LoC). The action owns all * the math (snap + linked cascade + live-drag mesh offsets + - * single-undo dance on commit). The wrapper just renders the cursor - * sphere and tracks its position from the live-transform store. + * single-undo dance on commit). The wrapper renders the cursor sphere + * tracking its position from the live-transform store. + * + * Selector stability: `originalCenter` is memoized so the live-transform + * fallback doesn't return a new array per render — that pattern blows + * up zustand's `Object.is` check and trips "getSnapshot result not + * cached" → infinite re-render. Same recipe in slab/ceiling move-tool. * * Mounted by ToolManager when `useEditor.movingNode` is a fence * (capability-driven dispatch — fence has no `movable` capability, so @@ -20,19 +26,19 @@ import { moveFenceDragAction } from './actions/move' */ export const FenceMoveTool: React.FC<{ node: FenceNode }> = ({ node }) => { const fenceId = node.id - const originalCenter: [number, number, number] = [ - (node.start[0] + node.end[0]) / 2, - 0, - (node.start[1] + node.end[1]) / 2, - ] + const originalCenter: [number, number, number] = useMemo( + () => [(node.start[0] + node.end[0]) / 2, 0, (node.start[1] + node.end[1]) / 2], + [node.start, node.end], + ) - // Live position from the live-transforms store — the action writes - // here every preview tick (live-drag exception). Falls back to the - // original center until the first move. - const liveCenter = useLiveTransforms((s) => { - const t = s.get(fenceId) - return t?.position ?? originalCenter - }) + // Subscribe to the live-transform reference only (stable across + // renders unless set/clear was called). Derive position via useMemo + // so the selector itself stays cached. + const liveTransform = useLiveTransforms((s) => s.get(fenceId)) + const liveCenter: [number, number, number] = useMemo( + () => liveTransform?.position ?? originalCenter, + [liveTransform, originalCenter], + ) const exitMoveMode = (committed: boolean) => { if (committed) triggerSFX('sfx:item-place') @@ -53,7 +59,7 @@ export const FenceMoveTool: React.FC<{ node: FenceNode }> = ({ node }) => { return ( - + ) } diff --git a/packages/nodes/src/slab/actions/move.ts b/packages/nodes/src/slab/actions/move.ts index c1203ea0..6719ad78 100644 --- a/packages/nodes/src/slab/actions/move.ts +++ b/packages/nodes/src/slab/actions/move.ts @@ -8,7 +8,11 @@ import { useScene, type WallNode, } from '@pascal-app/core' -import { type FencePlanPoint, snapFenceDraftPoint } from '@pascal-app/editor' +import { type FencePlanPoint, snapFenceDraftPoint, triggerSFX } from '@pascal-app/editor' + +function sameSnap(a: FencePlanPoint | null, b: FencePlanPoint): boolean { + return a !== null && a[0] === b[0] && a[1] === b[1] +} /** * Phase 5 Stage D — whole-slab move drag affordance. @@ -16,7 +20,8 @@ import { type FencePlanPoint, snapFenceDraftPoint } from '@pascal-app/editor' * Translates the slab's boundary polygon (and any holes) rigidly under * the pointer. Snaps to walls / fences / grid at the level. Latches * the drag anchor on the first preview tick so the slab doesn't jump - * to wherever the activation click landed. + * to wherever the activation click landed. Emits grid-snap sfx when + * the snapped position changes between ticks (matches legacy UX). * * Unlike fence move, the slab port does **not** use the live-drag * exception — polygon CSG geometry is expensive to rebuild per frame, @@ -52,6 +57,7 @@ export type MoveSlabCtx = { levelWalls: WallNode[] levelFences: FenceNode[] dragAnchor: FencePlanPoint | null + lastSnapped: FencePlanPoint | null } export type MoveSlabDraft = { @@ -98,6 +104,7 @@ export const moveSlabDragAction: DragAction = { levelWalls, levelFences, dragAnchor: null, + lastSnapped: null, } }, @@ -107,6 +114,11 @@ export const moveSlabDragAction: DragAction = { walls: ctx.levelWalls, fences: ctx.levelFences, }) + // Emit grid-snap sfx when the snapped position changes. + if (!sameSnap(ctx.lastSnapped, snapped)) { + if (ctx.lastSnapped !== null) triggerSFX('sfx:grid-snap') + ctx.lastSnapped = snapped + } if (!ctx.dragAnchor) ctx.dragAnchor = snapped const deltaX = snapped[0] - ctx.dragAnchor[0] const deltaZ = snapped[1] - ctx.dragAnchor[1] @@ -130,11 +142,9 @@ export const moveSlabDragAction: DragAction = { }, commit: (draft, ctx, scene) => { - if (draft.deltaX === 0 && draft.deltaZ === 0) return false - - // Single-undo dance — revert via snapshot, resume history, re-apply - // the final polygon/holes. Zundo captures the whole drag as one - // Ctrl-Z step. + // Always push — see fence/actions/curve.ts. Even on a no-movement + // commit, the dance must push a pastState entry so Ctrl-Z doesn't + // cancel whatever was on the stack before activation. scene.restoreAll() scene.resumeHistory() scene.update(ctx.slabId, { diff --git a/packages/nodes/src/slab/move-tool.tsx b/packages/nodes/src/slab/move-tool.tsx index 086127da..665bd714 100644 --- a/packages/nodes/src/slab/move-tool.tsx +++ b/packages/nodes/src/slab/move-tool.tsx @@ -3,32 +3,42 @@ import { type SlabNode, useScene } from '@pascal-app/core' import { CursorSphere, triggerSFX, useDragAction, useEditor } from '@pascal-app/editor' import { useViewer } from '@pascal-app/viewer' +import { useMemo } from 'react' import { moveSlabDragAction } from './actions/move' /** * Phase 5 Stage D — thin React wrapper around `moveSlabDragAction`. * * Replaces the legacy `MoveSlabTool` (182 LoC). All math + history - * dance lives in the action; this wrapper just renders the cursor - * sphere following the live polygon center. + * dance lives in the action; this wrapper renders the cursor sphere + * at the live polygon center. + * + * NOTE on selector stability: the live polygon center MUST be derived + * via `useMemo` over the node reference rather than computed inside + * the `useScene` selector — returning a fresh `[x, z]` tuple from the + * selector on every call triggers "getSnapshot result not cached" + * → infinite re-render. Same pattern in fence/ceiling move-tool. */ export const SlabMoveTool: React.FC<{ node: SlabNode }> = ({ node }) => { const slabId = node.id + const initialCenter: [number, number] = useMemo(() => { + if (node.polygon.length === 0) return [0, 0] + let sx = 0 + let sz = 0 + for (const [x, z] of node.polygon) { + sx += x + sz += z + } + return [sx / node.polygon.length, sz / node.polygon.length] + }, [node.polygon]) - const initialCenter: [number, number] = - node.polygon.length > 0 - ? [ - node.polygon.reduce((s, [x]) => s + x, 0) / node.polygon.length, - node.polygon.reduce((s, [, z]) => s + z, 0) / node.polygon.length, - ] - : [0, 0] - - // Live polygon center — re-derived from the scene store every tick - // since the action writes the translated polygon onto the slab. - const liveCenter = useScene((s) => { - const live = s.nodes[slabId] - if (live?.type !== 'slab') return initialCenter - const poly = (live as SlabNode).polygon + // Subscribe to the live node reference (stable across renders when + // the node hasn't changed; new reference per scene update). Derive + // the center inside `useMemo` so the selector itself stays cached. + const liveNode = useScene((s) => s.nodes[slabId]) + const liveCenter = useMemo<[number, number]>(() => { + if (liveNode?.type !== 'slab') return initialCenter + const poly = (liveNode as SlabNode).polygon if (poly.length === 0) return initialCenter let sx = 0 let sz = 0 @@ -36,8 +46,8 @@ export const SlabMoveTool: React.FC<{ node: SlabNode }> = ({ node }) => { sx += x sz += z } - return [sx / poly.length, sz / poly.length] as [number, number] - }) + return [sx / poly.length, sz / poly.length] + }, [liveNode, initialCenter]) const exitMoveMode = (committed: boolean) => { if (committed) triggerSFX('sfx:item-place') diff --git a/packages/nodes/src/wall/actions/curve.ts b/packages/nodes/src/wall/actions/curve.ts index 27fa6d59..836689e0 100644 --- a/packages/nodes/src/wall/actions/curve.ts +++ b/packages/nodes/src/wall/actions/curve.ts @@ -68,7 +68,9 @@ export const curveWallDragAction: DragAction = { }, commit: (draft, ctx, scene) => { - if (draft.curveOffset === ctx.originalCurveOffset) return false + // Always push a pastState entry — see fence/actions/curve.ts for + // the rationale (no-op-bend would otherwise let Ctrl-Z cancel the + // wall creation). scene.restoreAll() scene.resumeHistory() scene.update(ctx.nodeId, { curveOffset: draft.curveOffset } as Partial)