From 8eab9979ab7a6cb7c5b101735f598cd304f4d05d Mon Sep 17 00:00:00 2001 From: Open Pascal Date: Tue, 16 Jun 2026 19:56:31 +0000 Subject: [PATCH] fix: address 2nd-round adversarial review of PR #402 fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - core: decouple drag-follow from distributionRole — add portConnectivityFollow flag to NodeDefinition; pipe-trap opts out (portConnectivityFollow: false) so dragging a connected pipe endpoint stretches the trap arm instead of yanking the anchored trap fixture - core: remove the module-level getLevelHeight cache entirely — it was keyed only by nodes-object identity, which could return stale heights for in-place mutations by pure/headless callers. The function is now fully pure and deterministic; viewer hot path recomputes per frame as before (the cache only ever skipped the resolver-free branch) - test: harden port-connectivity-pipe.test.ts — real DuctSegmentNode cross-family isolation case (was waste-vs-vent), new pipe-trap anchor case (run drag doesn't move trap; trap drag still stretches run), and beforeEach/afterEach registry reset instead of leaky beforeAll - nit: biome format/import-order on all touched files --- packages/core/src/registry/types.ts | 14 ++ packages/core/src/services/level-height.ts | 28 +--- .../core/src/services/port-connectivity.ts | 4 + packages/nodes/src/duct-segment/tool.tsx | 6 +- packages/nodes/src/pipe-trap/definition.ts | 1 + .../src/shared/port-connectivity-pipe.test.ts | 153 ++++++++++-------- .../viewer/src/systems/level/level-system.tsx | 6 +- .../viewer/src/systems/level/level-utils.ts | 6 +- 8 files changed, 121 insertions(+), 97 deletions(-) diff --git a/packages/core/src/registry/types.ts b/packages/core/src/registry/types.ts index f171295f..1afd7d41 100644 --- a/packages/core/src/registry/types.ts +++ b/packages/core/src/registry/types.ts @@ -720,6 +720,20 @@ export type NodeDefinition> = { * Kinds outside any distribution system leave this unset. */ distributionRole?: DistributionRole + /** + * When `distributionRole` is `'fitting'`, controls whether this fitting + * is dragged as a rigid follower when a connected run endpoint moves. + * + * - `true` (default for `distributionRole === 'fitting'`): the fitting + * translates rigidly so its mated collar stays on the moved port — the + * right behaviour for in-line fittings (elbows, tees, wyes, crosses). + * - `false`: the fitting is anchored in space; moving a connected run + * endpoint stretches the run arm, not the fitting. Use this for + * fixed-position fixtures like `pipe-trap`. + * + * Has no effect when `distributionRole` is not `'fitting'`. + */ + portConnectivityFollow?: boolean defaults: () => Omit, 'id' | 'type'> migrate?: Record unknown> diff --git a/packages/core/src/services/level-height.ts b/packages/core/src/services/level-height.ts index 29d68d26..46f4da0a 100644 --- a/packages/core/src/services/level-height.ts +++ b/packages/core/src/services/level-height.ts @@ -7,40 +7,18 @@ export const DEFAULT_LEVEL_HEIGHT = 2.5 * Optional resolver for a wall's rendered base Y (mesh elevation). * * `packages/core` is pure domain logic and must not read viewer/Three.js - * state (see AGENTS.md "Layer Boundaries"). Callers that legitimately have + * state (see AGENTS.md “Layer Boundaries”). Callers that legitimately have * registry access (viewer systems, node tools) may pass a resolver so the * mesh elevation is factored in; pure/headless callers (MCP, tests, server) * omit it and get a deterministic result from serialized node data alone. */ export type WallBaseYResolver = (wallId: AnyNodeId) => number | undefined -// Cache: levelId → computed height. Invalidated when the nodes reference changes. -// Zustand produces a new `nodes` object on every mutation, so reference equality -// is a zero-cost way to detect stale data without any subscription overhead. -// -// NOTE: the cache is keyed only by the `nodes` reference, so it is only safe to -// reuse across calls that pass the same resolver semantics. When a resolver is -// supplied (viewer path), mesh Y can change without a `nodes` reference change, -// so the viewer caller must not rely on the cache for live values — it passes a -// resolver and we recompute. We therefore only cache resolver-free (pure) -// results, which are a deterministic function of `nodes`. -const heightCache = new Map() -let lastNodesRef: object | null = null - export function getLevelHeight( levelId: string, nodes: Record, resolveWallBaseY?: WallBaseYResolver, ): number { - if (nodes !== lastNodesRef) { - heightCache.clear() - lastNodesRef = nodes - } - - // Only the pure (resolver-free) computation is cacheable: with a resolver the - // result can depend on live mesh state that the `nodes` reference doesn't track. - if (!resolveWallBaseY && heightCache.has(levelId)) return heightCache.get(levelId)! - const level = nodes[levelId as LevelNode['id']] as LevelNode | undefined if (!level) return DEFAULT_LEVEL_HEIGHT @@ -60,7 +38,5 @@ export function getLevelHeight( } } - const height = maxTop > 0 ? maxTop : DEFAULT_LEVEL_HEIGHT - if (!resolveWallBaseY) heightCache.set(levelId, height) - return height + return maxTop > 0 ? maxTop : DEFAULT_LEVEL_HEIGHT } diff --git a/packages/core/src/services/port-connectivity.ts b/packages/core/src/services/port-connectivity.ts index ac14e9f3..3dbbbf35 100644 --- a/packages/core/src/services/port-connectivity.ts +++ b/packages/core/src/services/port-connectivity.ts @@ -112,8 +112,12 @@ export function analyzePortConnectivity( // Generalised across every distribution family (HVAC duct + DWV pipe): // `run` partners stretch an endpoint, `fitting` partners follow rigidly. // Terminals/equipment mount to surfaces and are intentionally NOT dragged. + // Fittings that declare `portConnectivityFollow: false` are anchored + // fixtures (e.g. pipe-trap) — moving a connected run stretches the arm. const otherRole = roleOf(other) if (otherRole !== 'run' && otherRole !== 'fitting') continue + const otherDef = nodeRegistry.get(other.type) + if (otherRole === 'fitting' && otherDef?.portConnectivityFollow === false) continue const otherPorts = portsOf(other) if (!otherPorts) continue diff --git a/packages/nodes/src/duct-segment/tool.tsx b/packages/nodes/src/duct-segment/tool.tsx index df0a778f..eb8ca57d 100644 --- a/packages/nodes/src/duct-segment/tool.tsx +++ b/packages/nodes/src/duct-segment/tool.tsx @@ -552,8 +552,10 @@ const DuctSegmentTool = () => { // ceiling (centerline = ceiling height − radius). const resolveBaseY = (): number => { if (!ceilingModeRef.current) return 0 - const ceiling = getLevelHeight(activeLevelId, useScene.getState().nodes, (wallId) => - sceneRegistry.nodes.get(wallId)?.position.y, + const ceiling = getLevelHeight( + activeLevelId, + useScene.getState().nodes, + (wallId) => sceneRegistry.nodes.get(wallId)?.position.y, ) const p = profileRef.current const verticalIn = p.shape === 'round' ? p.diameter : p.height diff --git a/packages/nodes/src/pipe-trap/definition.ts b/packages/nodes/src/pipe-trap/definition.ts index 331bf8e1..541afb06 100644 --- a/packages/nodes/src/pipe-trap/definition.ts +++ b/packages/nodes/src/pipe-trap/definition.ts @@ -17,6 +17,7 @@ export const pipeTrapDefinition: NodeDefinition = { schema: PipeTrapNode, category: 'utility', distributionRole: 'fitting', + portConnectivityFollow: false, // trap is anchored; dragging a connected run stretches the arm, not the trap defaults: () => ({ object: 'node', diff --git a/packages/nodes/src/shared/port-connectivity-pipe.test.ts b/packages/nodes/src/shared/port-connectivity-pipe.test.ts index 3e1eb3cf..5590832e 100644 --- a/packages/nodes/src/shared/port-connectivity-pipe.test.ts +++ b/packages/nodes/src/shared/port-connectivity-pipe.test.ts @@ -1,64 +1,77 @@ -import { beforeAll, describe, expect, test } from 'bun:test' +import { afterEach, beforeEach, describe, expect, test } from 'bun:test' import { - analyzePortConnectivity, type AnyNode, + analyzePortConnectivity, + DuctSegmentNode, loadPlugin, nodeRegistry, PipeFittingNode, PipeSegmentNode, + PipeTrapNode, resolveConnectivityUpdates, } from '@pascal-app/core' import { builtinPlugin } from '../index' +type Port = { id: string; position: [number, number, number] } + +function portsOf(kind: string, node: AnyNode): ReadonlyArray { + return nodeRegistry.get(kind)!.ports!(node) as ReadonlyArray +} + +function wasteTee(): PipeFittingNode { + return PipeFittingNode.parse({ + object: 'node', + parentId: null, + visible: true, + metadata: {}, + fittingType: 'sanitary-tee', + diameter: 2, + diameter2: 2, + pipeMaterial: 'pvc', + system: 'waste', + position: [0, 0, 0], + rotation: [0, 0, 0], + }) +} + +function pipeRunFrom(point: [number, number, number], system: 'waste' | 'vent' = 'waste') { + return PipeSegmentNode.parse({ + object: 'node', + parentId: null, + visible: true, + metadata: {}, + diameter: 2, + pipeMaterial: 'pvc', + system, + path: [point, [point[0] + 3, point[1], point[2]]], + }) +} + /** * Regression coverage for the generalized (HVAC duct + DWV pipe) * port-connectivity service. Before PR #402's follow-up fix the service * only tracked `duct-segment` / `duct-fitting`, so moving a `pipe-fitting` * left attached `pipe-segment` endpoints behind. These tests assert the - * role-based generalization carries pipe runs along. + * role-based generalization carries pipe runs along without fusing unrelated + * systems or anchored trap fixtures. */ describe('port connectivity — DWV pipe family', () => { - beforeAll(async () => { + beforeEach(async () => { nodeRegistry._reset() await loadPlugin(builtinPlugin) }) + afterEach(() => { + nodeRegistry._reset() + }) + test('moving a pipe-fitting stretches the connected pipe-segment endpoint', () => { // A sanitary tee at the origin; its run ports sit on ±X at the hub legs. - const fitting = PipeFittingNode.parse({ - object: 'node', - parentId: null, - visible: true, - metadata: {}, - fittingType: 'sanitary-tee', - diameter: 2, - diameter2: 2, - pipeMaterial: 'pvc', - system: 'waste', - position: [0, 0, 0], - rotation: [0, 0, 0], - }) - - const fittingPorts = nodeRegistry.get('pipe-fitting')!.ports!(fitting) as ReadonlyArray<{ - id: string - position: [number, number, number] - }> - const outlet = fittingPorts.find((p) => p.id === 'outlet')! + const fitting = wasteTee() + const outlet = portsOf('pipe-fitting', fitting as AnyNode).find((p) => p.id === 'outlet')! // A pipe run whose START port coincides with the fitting's outlet collar. - const run = PipeSegmentNode.parse({ - object: 'node', - parentId: null, - visible: true, - metadata: {}, - diameter: 2, - pipeMaterial: 'pvc', - system: 'waste', - path: [ - [outlet.position[0], outlet.position[1], outlet.position[2]], - [outlet.position[0] + 3, outlet.position[1], outlet.position[2]], - ], - }) + const run = pipeRunFrom(outlet.position) const nodes: Record = { [fitting.id]: fitting as AnyNode, @@ -84,45 +97,55 @@ describe('port connectivity — DWV pipe family', () => { }) test('incompatible systems do not fuse (a supply duct is not dragged by a waste fitting)', () => { - const fitting = PipeFittingNode.parse({ - object: 'node', - parentId: null, - visible: true, - metadata: {}, - fittingType: 'sanitary-tee', - diameter: 2, - diameter2: 2, - pipeMaterial: 'pvc', - system: 'waste', - position: [0, 0, 0], - rotation: [0, 0, 0], - }) - const fittingPorts = nodeRegistry.get('pipe-fitting')!.ports!(fitting) as ReadonlyArray<{ - id: string - position: [number, number, number] - }> - const outlet = fittingPorts.find((p) => p.id === 'outlet')! + const fitting = wasteTee() + const outlet = portsOf('pipe-fitting', fitting as AnyNode).find((p) => p.id === 'outlet')! - // A vent pipe sharing the same point but a different system. - const ventRun = PipeSegmentNode.parse({ + // A supply duct sharing the same point but a different distribution system. + const duct = DuctSegmentNode.parse({ object: 'node', parentId: null, visible: true, metadata: {}, - diameter: 2, - pipeMaterial: 'pvc', - system: 'vent', - path: [ - [outlet.position[0], outlet.position[1], outlet.position[2]], - [outlet.position[0] + 3, outlet.position[1], outlet.position[2]], - ], + diameter: 6, + ductMaterial: 'flex', + system: 'supply', + path: [outlet.position, [outlet.position[0] + 3, outlet.position[1], outlet.position[2]]], }) const nodes: Record = { [fitting.id]: fitting as AnyNode, - [ventRun.id]: ventRun as AnyNode, + [duct.id]: duct as AnyNode, } const connectivity = analyzePortConnectivity(fitting as AnyNode, nodes) - expect(connectivity.connections.find((c) => c.nodeId === ventRun.id)).toBeUndefined() + expect(connectivity.connections.find((c) => c.nodeId === duct.id)).toBeUndefined() + }) + + test('pipe-trap is anchored when a connected pipe endpoint moves', () => { + const trap = PipeTrapNode.parse({ + object: 'node', + parentId: null, + visible: true, + metadata: {}, + position: [0, 0, 0], + rotation: 0, + diameter: 1.5, + pipeMaterial: 'pvc', + armLengthM: 0, + }) + const outlet = portsOf('pipe-trap', trap as AnyNode).find((p) => p.id === 'outlet')! + const run = pipeRunFrom(outlet.position) + + const nodes: Record = { + [trap.id]: trap as AnyNode, + [run.id]: run as AnyNode, + } + + // Moving the run endpoint must not translate the fixed-position trap. + const runConnectivity = analyzePortConnectivity(run as AnyNode, nodes) + expect(runConnectivity.connections.find((c) => c.nodeId === trap.id)).toBeUndefined() + + // Moving the trap itself still stretches the connected run endpoint. + const trapConnectivity = analyzePortConnectivity(trap as AnyNode, nodes) + expect(trapConnectivity.connections.find((c) => c.nodeId === run.id)).toBeDefined() }) }) diff --git a/packages/viewer/src/systems/level/level-system.tsx b/packages/viewer/src/systems/level/level-system.tsx index ac957051..63877873 100644 --- a/packages/viewer/src/systems/level/level-system.tsx +++ b/packages/viewer/src/systems/level/level-system.tsx @@ -39,8 +39,10 @@ export const LevelSystem = () => { obj.position.y = lerp(obj.position.y, targetY, delta * 12) // Smoothly animate to new Y position obj.visible = levelMode !== 'solo' || level?.id === selectedLevel || !selectedLevel - cumulativeY += getLevelHeight(levelId, nodes, (wallId) => - sceneRegistry.nodes.get(wallId)?.position.y, + cumulativeY += getLevelHeight( + levelId, + nodes, + (wallId) => sceneRegistry.nodes.get(wallId)?.position.y, ) } }, 5) // Using a lower priority so it runs after transforms from other systems have settled diff --git a/packages/viewer/src/systems/level/level-utils.ts b/packages/viewer/src/systems/level/level-utils.ts index cb5af5ad..bf595c6c 100644 --- a/packages/viewer/src/systems/level/level-utils.ts +++ b/packages/viewer/src/systems/level/level-utils.ts @@ -41,8 +41,10 @@ export function snapLevelsToTruePositions(): () => void { for (const { levelId, obj } of entries) { obj.position.y = cumulativeY obj.visible = true - cumulativeY += getLevelHeight(levelId, nodes, (wallId) => - sceneRegistry.nodes.get(wallId)?.position.y, + cumulativeY += getLevelHeight( + levelId, + nodes, + (wallId) => sceneRegistry.nodes.get(wallId)?.position.y, ) }