review-architecture skill | PlugMCP

review-architecture skill | PlugMCP

geometry

renderer

system

# If the user gave a PR URL or number:
gh pr diff <pr-number-or-url>

# If reviewing the current branch:
git diff main...HEAD

gh pr view <pr> --json files --jq '.files[].path'
# or
git diff --name-only main...HEAD

Three independent fields: def.geometry?: (node, ctx) => Object3D, def.renderer?: () => Promise<{ default }>, def.system?: () => Promise<{ default }>. There is no discriminator — presence is participation. Setting all three is fine if the kind genuinely needs them; setting a def.system whose only job is to rebuild geometry on dirty is a smell — collapse to def.geometry and let <GeometrySystem> do the work.
Builders must be pure. A def.geometry function must not import useScene, must not mutate the store, and must not depend on React context. Read other nodes via GeometryContext (ctx.resolve / ctx.children / ctx.siblings / ctx.parent).
Builders emit local-space children. <ParametricNodeRenderer> binds <group position={liveTransform?.position ?? node.position}> in JSX. A builder that bakes world position into vertex coords, or a system that imperatively writes group.position / group.rotation, will desync R3F's prop binding — the node will snap to (0,0,0) after rebuild. Flag any imperative group.position.set(...) inside def.geometry or a registered system. (Tool-driven sceneRegistry.nodes.get(id).position.set(...) during a live drag is fine and is the documented pattern — see hook hygiene below.)
Tag geometry-built children. <GeometrySystem> only disposes children carrying userData.__fromGeometry = true. Custom systems that imperatively add children to a registered group must follow the same convention if the group can host React-mounted children (e.g. shelf surfaces hosting items).
One registered mesh per node ID. If a custom renderer mounts multiple objects, register the parent group (or whichever object the system needs to address via sceneRegistry.nodes.get(id)).
Previews must clone cached materials. If def.preview calls the geometry builder and then sets material.opacity = 0.5, but the builder caches materials at module scope (most do, keyed on material / materialPreset), the mutation leaks into every committed instance. Clone, mutate the clone, reassign mesh.material, dispose only the clone on unmount. Reference: nodes/src/shelf/preview.tsx.
Schema changes must keep old scenes loadable. Any diff that adds, renames, removes, or retypes a property on a node schema needs a load path for scenes saved before the change (parsed through AnyNode in SceneState.setScene). A new field needs a Zod .default() / .optional(). A rename / removal / retype needs a migrateNodes entry in packages/core/src/store/use-scene.ts that rewrites the legacy shape before parse — a .default() alone silently drops the old value. A schema diff that does neither is a blocker: it breaks every existing scene. See wiki/architecture/node-schemas.md § Schema Evolution.
Host kinds need children on the schema. If def.relations.hosts is set, the schema must declare children: z.array(z.string()).default([]) (and migrateNodes must patch existing scenes). Otherwise useScene.createNode(child, parentId) writes a parent.children entry into nothing and the host never sees the new child.
Movable opt-in. MoveTool dispatches to MoveRegistryNodeTool only when def.capabilities.movable is set. Kinds with bespoke move semantics (wall endpoint drag with linked-wall cascade, slab vertex edit, etc.) deliberately omit movable and supply def.affordanceTools.move instead. Force-routing a bespoke-move kind through generic dispatch (nodeRegistry.has(kind) instead of def.capabilities.movable) is a regression — call it out. See wiki/architecture/node-definitions.md § "Opting out of a generic path".
Paint dispatch lives on def.capabilities.paint. A paintable kind declares resolveRole / buildPatch / applyPreview (+ optional getEffectiveMaterial) on PaintCapability; the editor's selection-manager routes hover / click / preview through the generic dispatcher. A PR that adds an if (node.type === '<kind>') arm to paint-mode handling, paint-preview application, or material picker resolution is a regression — the behaviour belongs on the kind's paint capability. See packages/core/src/registry/types.ts (PaintCapability).
Slots + world-scale UVs for paintable surfaces. A new kind (or geometry change) that exposes paintable parts must follow the unified slot convention, not reinvent it:
- Paintable parts are slots, carried on the node. Overrides live in a slots record (slotId → MaterialRef, scene:/library:) on the schema, resolved via def.capabilities.paint — not ad-hoc per-surface material / materialPreset fields, and not a parallel store. A new paintable kind whose schema lacks slots (or whose duplicate / preset / clone path drops it) is a blocker: it silently loses painted materials. (Slots are plain data — generic clone/parse preserves them; bespoke draft-rebuild placement paths must thread them through explicitly. Reference bug: item duplicate rebuilt the draft from asset and dropped slots.)
- Texturable geometry emits UVs in metres (1 UV unit = 1 m). Any def.geometry producing a surface a finish can tile onto must generate UVs at the same world scale walls / slabs / roofs use, because catalog finishes set repeat as tiles-per-metre. Unitless, bounding-box-normalised, or hardcoded UVs that don't scale with the surface are a blocker — finishes won't tile consistently. Flat-colour-only surfaces need no UVs. GLB item authoring follows the same contract via slot_-prefixed materials (case-insensitive, slot_ → slot id). See wiki/architecture/materials-and-themes.md § "Texture world scale" and wiki/architecture/item-authoring.md.
Floor elevation lives on def.capabilities.floorPlaced. Kinds that rest on a level and lift over overlapping slabs declare a footprint (and optional applies predicate) on FloorPlacedConfig; the generic <FloorElevationSystem> writes slabElevation + node.position[1] onto the registered mesh on each dirty mark. A new per-kind useEffect / per-kind system that recomputes Y from slab overlap is a regression — the per-kind block was lifted out of ItemSystem in Phase 6.1. See packages/core/src/registry/types.ts (FloorPlacedConfig).
Render-mode behaviour goes through def.surfaceRole. Solid / Rendered / Clay viewer modes look up the kind's surfaceRole on NodeDefinition to pick the right material strategy (clay overrides, edge passes, theme overlays). New if (node.type === '<kind>') arms inside the render-mode pipeline or theme application are a regression — the behaviour belongs on def.surfaceRole. See packages/core/src/registry/types.ts:529.
Capability names must describe verbs, not host kinds. movable / paint / floorPlaced / cuts / selectable describe what the node does. A new capability named after a host kind (slabAccessory, wallAccessory, siteAccessory, anything Xaccessory / Xhosted shaped) couples the registry's type surface to one specific host and reads as precedent for the next reviewer. Blocker. Push back: generalise into a paired host-side capability ("I merge subtractive accessories from my children") + accessory-side capability ("I provide a cut geometry, cascade my dirty mark to my host's parent"); cuts go through capabilities.cuts (cut intents, packages/core/src/schema/cut.ts). The single existing case — capabilities.roofAccessory (packages/core/src/registry/types.ts:791, consumed by packages/viewer/src/systems/roof/roof-system.tsx) — is documented tech debt; do not extend the pattern.
Per-kind files in legacy locations are a regression. New viewer/src/components/renderers/<kind>/*, viewer/src/systems/<kind>-system.tsx, editor/src/components/tools/<kind>/*, editor/src/components/ui/panels/<kind>-panel.tsx, editor/src/components/ui/helpers/<kind>-helper.tsx, or inline useMemo floor-plan entry-builders inside editor/src/components/editor/floorplan-panel.tsx — all of these were systematically deleted at Phase 6. The behavior belongs on the kind's NodeDefinition (def.renderer / def.system / def.geometry / def.tool / def.affordanceTools / parametrics.customPanel / def.toolHints / def.floorplan).
Floor-plan output via def.floorplan. New per-kind floor-plan rendering must return FloorplanGeometry from def.floorplan(node, ctx) and be rendered by <FloorplanRegistryLayer>. New inline branches in floorplan-panel.tsx are a blocker.
Plugin contract surface. A PR that extends the v1 plugin surface — adding plugin.materials, plugin.systems, plugin.panels, or making plugins extend host stores (useScene / useEditor / useViewer) — is out of scope for the v1 contract documented in wiki/architecture/plugin-authoring.md. Either the change belongs as a new field on NodeDefinition (additive, doesn't bump apiVersion) or it needs its own plan.

Stores hold state + setters only. No business logic, side effects, async work, or derived computations inside the store definition.
Derived values belong in selectors or systems, not in the store body.
No cross-store coupling: a store's action should not call another store's actions inside itself.
New state added to useViewer must be presentation-only (selection, camera, level mode, display toggles). Editor-only state (active tool, phase, edit mode, paint preview, floorplan state) goes in useEditor.
Node code does not import useScene directly. A kind's geometry / system / tool should read and write through SceneApi (passed in by the framework) or GeometryContext. Direct useScene.getState() calls inside packages/nodes/src/<kind>/ are a smell — they bypass the registry's IoC point and make the code harder to test.
Live drag motion is imperative, not store-driven. Tools must not call useLiveTransforms.set(...) per grid:move tick to animate registered parametric kinds — the selector path doesn't reliably re-render and the mesh visibly disappears mid-drag. Use sceneRegistry.nodes.get(node.id)?.position.set(x, y, z) instead, and commit once at the end via useScene.temporal.getState().resume() → updateNode → pause(). The reference implementation is MoveRegistryNodeTool. This is the only sanctioned use of imperative mesh transforms by a tool; flag any other location that does the same.
Data-driven drags preview via useLiveNodeOverrides, never per-tick useScene. A kind whose geometry is recomputed from data fields (wall start/end, opening host-cut, endpoint reshape) previews by publishing field patches to useLiveNodeOverrides (merged by getEffectiveWall / getEffectiveNode), writing the scene store once on commit. A tool that calls useScene.updateNodes/updateNode on grid:move (or any per-pointer-move tick) is a blocker — it swaps the nodes map ref and re-renders every useScene(s => s.nodes) subscriber app-wide each frame. markDirty per tick is fine for bounded gestures (drag marks drain every frame); a useFrame/animation loop that marks dirty for as long as something animates is a blocker — the scene can then never settle to DIRTY 0. Animations signal rebuilds through their own records (useInteractive animations), marking dirty once on completion; see wiki/architecture/node-definitions.md § "geometry + system". Grep tell: updateNode(s)?( in an onGridMove/onMove/applyPreview path under packages/nodes/src/<kind>/. See wiki/architecture/tools.md § "Data-driven live drag".

No new useEditor interaction flag. "What the user is doing" is owned by useInteractionScope (begin / update / end / endIf). A new useEditor boolean for an in-flight interaction (moving…, curving…, dragging…, editing…, …InFlight) is a blocker — it goes through the scope. The legacy mirror flags are being retired, not extended.
Snapping is mode-driven; Shift is not a bypass. A tool / move-tool / selection file that reads event.shiftKey, event.nativeEvent?.shiftKey, or modifiers.shiftKey to bypass snapping (raw cursor, skip grid, skip angle) is a blocker — the convention is Shift = cycle the mode, Alt = force/free. Snap state must come from isGridSnapActive() / isMagneticSnapActive() / isAngleSnapActive(). Grep tell: shiftKey near a snap / step / projectToAngleLock / alignment expression in packages/nodes/src/<kind>/{tool,move-tool,selection}.tsx. (Shift for multi-select in select mode, or a documented topology opt-out, is fine — confirm which it is.)
No hardcoded, ungated grid step. A quantize that isn't gated on isGridSnapActive() — always useEditor.getState().gridSnapStep, or a constant WALL_GRID_STEP / 0.5 / getSegmentGridStep() applied unconditionally — ignores the active mode and is a blocker. The gated form is const step = isGridSnapActive() ? useEditor.getState().gridSnapStep : 0.
Snappable kinds declare snapProfile. A kind whose tool snaps but whose NodeDefinition omits snapProfile ('item' | 'structural') gets no contextual chip and the wrong default mode-set — flag it (suggestion, blocker if it ships a bespoke per-kind snapping switch instead).
Bespoke movers must not open a moving scope. useMovingNode() reads the scope, and tool-manager mounts the generic MoveRegistryNodeTool whenever it's non-null. A bespoke move-tool.tsx that calls begin(movingScope(...)) or setMovingNode(node) re-creates the dual-path double-handling (FPS collapse / teleport on move). Blocker. Mode-driven snapping inside a bespoke mover must resolve the mode without a global moving / reshaping scope (see interaction-scope.md § "Snapping mode & modifiers").
event.altKey is not an alignment bypass. A drafting/preview path that reads event.altKey to suppress Figma-alignment is a blocker in any new or touched tool — alignment follows the magnetic snap mode (bypass: !isMagneticSnapActive()). Alt is force/free for placement/move; it is not a snap/alignment modifier. The one sanctioned Alt use outside force is the wall/fence chain-mode toggle (clean Alt-tap → cycleWallChainMode / cycleFenceChainMode, via hooks/use-keyboard.ts isChainModeContext()), allowed only because wall/fence drafting has no force role. Grep tell: event.altKey near an align / bypass expression in a tool.tsx / floorplan preview path.
Known-legacy exceptions (migrate on touch). Tracked debt in wiki/architecture/interaction-scope.md (Known-legacy); a PR that touches one must migrate it, not extend it; a new tool on either legacy pattern is a blocker regardless. (1) shiftKey snap-bypass in the MEP move/endpoint tools (packages/nodes/src/{duct-segment,pipe-segment,liquid-line,lineset,duct-fitting}/{move-tool,selection}.tsx). (2) altKey alignment-bypass in the roof / polygon / slab pointer-move previews (components/editor/floorplan-panel.tsx) and the resolveSlabPlanPointSnap / resolveCeilingPlanPointSnap paths. Already migrated — do not regress: wall + fence drafting (both modifier patterns) and zone drafting (components/tools/zone/zone-tool.tsx — mode-driven grid/angle gates, no Shift bypass).