fix: address 2nd-round adversarial review of PR #402 fixes
- 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
This commit is contained in:
@@ -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<string, number>()
|
||||
let lastNodesRef: object | null = null
|
||||
|
||||
export function getLevelHeight(
|
||||
levelId: string,
|
||||
nodes: Record<AnyNodeId, AnyNode>,
|
||||
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
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user