Skip to content

Commit cefcb01

Browse files
authored
fix(editor): null-safe cursor-group access in placement coordinator (pascalorg#323)
Guards every `cursorGroupRef.current` dereference in the placement coordinator against the null window where mitt listeners are live but the `<group>` is unmounted (mount/teardown race) — the EDITOR-BC/BD crash family. `getContext()` falls back to the draft's rotation, so the validation/revalidate path (Shift keys, onKeyUp) is safe; only the cursor *writes* are guarded, so Escape/right-click cancel, Shift reset, leave-state cleanup and transition state still run unconditionally. Also guards `wallPreviewRef` inside `WallTool.stopDrafting()` (the double-click/cancel path that the earlier diff missed). Reimplemented against current main (the original branch conflicted with pascalorg#366 and used over-broad handler guards that silently dropped cancel/Shift). Verified: `tsc -p apps/editor` clean, biome clean on touched files.
1 parent 98eeb1d commit cefcb01

2 files changed

Lines changed: 68 additions & 37 deletions

File tree

packages/editor/src/components/tools/item/use-placement-coordinator.tsx

Lines changed: 65 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,9 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
345345
}
346346
if (!asset.attachTo && placementState.current.surface === 'floor') {
347347
gridPosition.current.y = 0
348-
cursorGroupRef.current.position.y = 0
348+
if (cursorGroupRef.current) {
349+
cursorGroupRef.current.position.y = 0
350+
}
349351
}
350352

351353
// ---- Helpers ----
@@ -356,7 +358,8 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
356358
draftItem: draftNode.current,
357359
gridPosition: gridPosition.current,
358360
state: { ...placementState.current },
359-
currentCursorRotationY: cursorGroupRef.current.rotation.y,
361+
currentCursorRotationY:
362+
cursorGroupRef.current?.rotation.y ?? draftNode.current?.rotation[1] ?? 0,
360363
})
361364

362365
const getActiveValidators = () =>
@@ -390,11 +393,13 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
390393
gridPosition.current.set(...result.gridPosition)
391394

392395
const c = worldToBuildingLocal(...result.cursorPosition)
393-
cursorGroupRef.current.position.set(c.x, c.y, c.z)
394-
if (result.cursorRotation) {
395-
cursorGroupRef.current.rotation.set(...result.cursorRotation)
396-
} else {
397-
cursorGroupRef.current.rotation.set(0, result.cursorRotationY, 0)
396+
if (cursorGroupRef.current) {
397+
cursorGroupRef.current.position.set(c.x, c.y, c.z)
398+
if (result.cursorRotation) {
399+
cursorGroupRef.current.rotation.set(...result.cursorRotation)
400+
} else {
401+
cursorGroupRef.current.rotation.set(0, result.cursorRotationY, 0)
402+
}
398403
}
399404

400405
const draft = draftNode.current
@@ -408,11 +413,13 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
408413
const ensureDraft = (result: TransitionResult) => {
409414
gridPosition.current.set(...result.gridPosition)
410415
const c = worldToBuildingLocal(...result.cursorPosition)
411-
cursorGroupRef.current.position.set(c.x, c.y, c.z)
412-
if (result.cursorRotation) {
413-
cursorGroupRef.current.rotation.set(...result.cursorRotation)
414-
} else {
415-
cursorGroupRef.current.rotation.set(0, result.cursorRotationY, 0)
416+
if (cursorGroupRef.current) {
417+
cursorGroupRef.current.position.set(c.x, c.y, c.z)
418+
if (result.cursorRotation) {
419+
cursorGroupRef.current.rotation.set(...result.cursorRotation)
420+
} else {
421+
cursorGroupRef.current.rotation.set(0, result.cursorRotationY, 0)
422+
}
416423
}
417424

418425
const initRotation: [number, number, number] = result.cursorRotation ?? [
@@ -453,20 +460,22 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
453460
const worldPos = new Vector3()
454461
mesh.getWorldPosition(worldPos)
455462
const localPos = worldToBuildingLocal(worldPos.x, worldPos.y, worldPos.z)
456-
cursorGroupRef.current.position.copy(localPos)
457-
if (draftNode.current.asset.attachTo) {
458-
// Wall/ceiling items: extract world Y rotation (handles wall-parented items correctly)
459-
const q = new Quaternion()
460-
mesh.getWorldQuaternion(q)
461-
cursorGroupRef.current.rotation.y = new Euler().setFromQuaternion(q, 'YXZ').y
462-
} else {
463-
// Floor items: the cursor group lives in building-local space, so use the
464-
// node's local Y rotation — the same value onGridMove applies. The world
465-
// quaternion would double-count any building rotation, leaving the initial
466-
// box mis-rotated until the first cursor move.
467-
cursorGroupRef.current.rotation.y = draftNode.current.rotation[1] ?? 0
463+
if (cursorGroupRef.current) {
464+
cursorGroupRef.current.position.copy(localPos)
465+
if (draftNode.current.asset.attachTo) {
466+
// Wall/ceiling items: extract world Y rotation (handles wall-parented items correctly)
467+
const q = new Quaternion()
468+
mesh.getWorldQuaternion(q)
469+
cursorGroupRef.current.rotation.y = new Euler().setFromQuaternion(q, 'YXZ').y
470+
} else {
471+
// Floor items: the cursor group lives in building-local space, so use the
472+
// node's local Y rotation — the same value onGridMove applies. The world
473+
// quaternion would double-count any building rotation, leaving the initial
474+
// box mis-rotated until the first cursor move.
475+
cursorGroupRef.current.rotation.y = draftNode.current.rotation[1] ?? 0
476+
}
468477
}
469-
} else {
478+
} else if (cursorGroupRef.current) {
470479
cursorGroupRef.current.position.copy(gridPosition.current)
471480
cursorGroupRef.current.rotation.y = draftNode.current.rotation[1] ?? 0
472481
}
@@ -486,6 +495,7 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
486495

487496
has3DPointerDrivenMoveRef.current = true
488497
lastRawPos.current.set(event.localPosition[0], event.localPosition[1], event.localPosition[2])
498+
if (!cursorGroupRef.current) return
489499
const result = floorStrategy.move(getContext(), event)
490500
if (!result) return
491501

@@ -530,7 +540,11 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
530540
if (!result) return
531541

532542
// Preserve cursor rotation for the next draft
533-
const currentRotation: [number, number, number] = [0, cursorGroupRef.current.rotation.y, 0]
543+
const currentRotation: [number, number, number] = [
544+
0,
545+
cursorGroupRef.current?.rotation.y ?? draftNode.current?.rotation[1] ?? 0,
546+
0,
547+
]
534548

535549
// Clear live transform before commit
536550
if (draftNode.current) {
@@ -581,6 +595,7 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
581595

582596
const onWallMove = (event: WallEvent) => {
583597
has3DPointerDrivenMoveRef.current = true
598+
if (!cursorGroupRef.current) return
584599
const ctx = getContext()
585600

586601
if (ctx.state.surface !== 'wall') {
@@ -761,7 +776,9 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
761776

762777
Object.assign(placementState.current, { surface: 'floor', surfaceItemId: null })
763778
gridPosition.current.set(wx, 0, wz)
764-
cursorGroupRef.current.position.set(wx, 0, wz)
779+
if (cursorGroupRef.current) {
780+
cursorGroupRef.current.position.set(wx, 0, wz)
781+
}
765782

766783
const draft = draftNode.current
767784
if (draft) {
@@ -795,6 +812,7 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
795812
const onItemMove = (event: ItemEvent) => {
796813
if (event.node.id === draftNode.current?.id) return
797814
has3DPointerDrivenMoveRef.current = true
815+
if (!cursorGroupRef.current) return
798816
const ctx = getContext()
799817

800818
if (ctx.state.surface !== 'item-surface') {
@@ -1019,6 +1037,7 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
10191037

10201038
const onCeilingMove = (event: CeilingEvent) => {
10211039
has3DPointerDrivenMoveRef.current = true
1040+
if (!cursorGroupRef.current) return
10221041
if (!draftNode.current && placementState.current.surface === 'ceiling') {
10231042
const nodes = useScene.getState().nodes
10241043
const setup = ceilingStrategy.enter(getContext(), event, resolveLevelId, nodes)
@@ -1254,7 +1273,9 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
12541273
draft.rotation = [currentRotation[0], newRotationY, currentRotation[2]]
12551274

12561275
// Ref + cursor mesh + item mesh — no store update during drag
1257-
cursorGroupRef.current.rotation.y = newRotationY
1276+
if (cursorGroupRef.current) {
1277+
cursorGroupRef.current.rotation.y = newRotationY
1278+
}
12581279
const mesh = sceneRegistry.nodes.get(draft.id)
12591280
if (mesh) mesh.rotation.y = newRotationY
12601281

@@ -1268,8 +1289,10 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
12681289
const z = snapToGrid(lastRawPos.current.z, swapDims ? dimX : dimZ)
12691290
gridPosition.current.set(x, gridPosition.current.y, z)
12701291
draft.position = [x, gridPosition.current.y, z]
1271-
cursorGroupRef.current.position.x = x
1272-
cursorGroupRef.current.position.z = z
1292+
if (cursorGroupRef.current) {
1293+
cursorGroupRef.current.position.x = x
1294+
cursorGroupRef.current.position.z = z
1295+
}
12731296
if (mesh) {
12741297
mesh.position.x = x
12751298
mesh.position.z = z
@@ -1292,21 +1315,26 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
12921315
worldSnapped.y,
12931316
worldSnapped.z,
12941317
)
1295-
cursorGroupRef.current.position.set(localSnapped.x, localSnapped.y, localSnapped.z)
1318+
if (cursorGroupRef.current) {
1319+
cursorGroupRef.current.position.set(localSnapped.x, localSnapped.y, localSnapped.z)
1320+
}
12961321
if (mesh) mesh.position.set(x, y, z)
12971322
}
12981323
}
12991324

13001325
// Update live transform for 2D floorplan with post-snap position
13011326
const currentLive = useLiveTransforms.getState().get(draft.id)
13021327
if (currentLive) {
1328+
const livePosition: [number, number, number] = cursorGroupRef.current
1329+
? [
1330+
cursorGroupRef.current.position.x,
1331+
cursorGroupRef.current.position.y,
1332+
cursorGroupRef.current.position.z,
1333+
]
1334+
: [draft.position[0], draft.position[1], draft.position[2]]
13031335
useLiveTransforms.getState().set(draft.id, {
13041336
...currentLive,
1305-
position: [
1306-
cursorGroupRef.current.position.x,
1307-
cursorGroupRef.current.position.y,
1308-
cursorGroupRef.current.position.z,
1309-
] as [number, number, number],
1337+
position: livePosition,
13101338
rotation: newRotationY,
13111339
})
13121340
}
@@ -1471,6 +1499,7 @@ export function usePlacementCoordinator(config: PlacementCoordinatorConfig): Rea
14711499
useFrame((_, delta) => {
14721500
if (!asset) return
14731501
if (!draftNode.current) return
1502+
if (!cursorGroupRef.current) return
14741503
// The mesh-position lerp below only makes sense once this coordinator
14751504
// owns the move via a 3D pointer event. Skip until then so that
14761505
// external drivers (e.g. the 2D `FloorplanRegistryMoveOverlay`

packages/nodes/src/wall/tool.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -431,7 +431,9 @@ export const WallTool: React.FC = () => {
431431

432432
const stopDrafting = () => {
433433
buildingState.current = 0
434-
wallPreviewRef.current.visible = false
434+
if (wallPreviewRef.current) {
435+
wallPreviewRef.current.visible = false
436+
}
435437
setDraftMeasurement(null)
436438
}
437439

0 commit comments

Comments
 (0)