Pascal Architecture PR Review

Architectural review for Pascal PRs. The user will provide a PR URL, branch name, or ask to review the current branch.

1. Load the rules (required — do not skip)

Read these before reviewing any diff. They are the source of truth, not your training data:

• wiki/architecture/layers.md
• wiki/architecture/systems.md — core systems vs viewer systems, what each may do
• wiki/architecture/renderers.md — renderer responsibilities and prohibitions
• wiki/architecture/tools.md — editor tools live only in apps/editor/components/tools/ or packages/nodes/src/<kind>/
• wiki/architecture/viewer-isolation.md — viewer must stay editor-agnostic
• wiki/architecture/node-definitions.md — the three-checkbox composition model (geometry / renderer / system)
• wiki/architecture/plugin-authoring.md — public contract for external node packs
• wiki/architecture/agent-surfaces.md — MCP ↔ hosted chat ↔ skill parity. Read whenever the diff touches packages/mcp/** or skills/**.

Required on every review. Read the remaining pages on demand when the diff touches their subject area:

• wiki/architecture/selection-managers.md
• wiki/architecture/scene-registry.md
• wiki/architecture/spatial-queries.md
• wiki/architecture/node-schemas.md
• wiki/architecture/inspector-field-limits.md — no arbitrary min/max on dimension fields. Read whenever the diff adds or edits parametrics.ts, a kind panel.tsx, or <SliderControl> bounds.
• wiki/architecture/events.md
• wiki/architecture/interaction-scope.md — the interaction state machine + the unified snapping/modifier convention. Read whenever the diff touches a tool, a move-tool / selection / endpoint / reshape file, lib/interaction/**, lib/snapping-mode.ts, or use-interaction-scope.

If anything in the diff looks like a new dispatch surface or registry concept, also read DECISIONS.md E-002 and wiki/architecture/node-definitions.md — they own the composition model and the capability vocabulary.

2. Fetch the diff

# 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

Also list changed files so you can map each to the relevant rule:

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

3. Layer classification — do this BEFORE the checklist

For every new file, new type, new store field, or new exported helper introduced by the diff, answer one question: which package does this belong to — core, viewer, editor, or nodes? If the answer is "editor" but the code lives in packages/core or packages/viewer (or vice versa), or if kind-specific code lands anywhere other than packages/nodes/src/<kind>/, flag it as a blocker. This is the most common and most damaging class of violation, and the checklist below won't reliably catch it on its own — do this pass explicitly.

The four packages and what they own

packages/core — domain data + pure logic. Owns: node schemas, the scene store (useScene), live transforms store, core systems (wall mitering, slab polygons, space detection), event bus, plain 2D/3D math helpers, sceneRegistry, the registry primitives (nodeRegistry, registerNode, loadPlugin, discoverPlugins/setPluginDiscovery, SceneApi, Plugin/NodeDefinition types). Consumed by every downstream package, including read-only embeds. Must not know about: Three.js/R3F, packages/viewer, apps/editor, packages/nodes, any rendering or UI concept, any tool/mode/phase concept, or any view-specific concept (floorplan, paint preview, cursor indicators, selection outline styling).

packages/viewer — the 3D canvas, shippable standalone. Owns: <Viewer>, the generic <NodeRenderer> / <ParametricNodeRenderer> / <GeometrySystem> / <RegisteredSystems> / <FloorplanRegistryLayer> plumbing, viewer systems (cutouts, zones, level positions, scans), the viewer store (useViewer) for genuine presentation state only (selection path, camera/level/wall/view modes, theme, display toggles, hover id), useNodeEvents. Consumed by both the editor and the read-only /viewer/[id] route. Must not know about: editor state (useEditor, tools, phases, modes), editor-only names baked into presentation modes ('delete', 'paint-ready'), editor-only state types (material preview, active paint target, floorplan anything), packages/nodes.

packages/editor (and apps/editor) — the editing experience. Owns: the tool framework (useDragAction, ParametricInspector, <MoveRegistryNodeTool>, the registry-aware dispatchers in tool-manager.tsx / MoveTool / panel-manager.tsx / helper-manager.tsx), useEditor, action menus, panels, the floorplan panel and its helpers, paint mode, selection-manager phase/mode logic, cursor badges, command palette, keyboard shortcuts — anything absent from the read-only viewer route. Injects itself into <Viewer> via children and props, never the reverse. Must not import from packages/nodes.

packages/nodes — the built-in plugin (pascal:core). Owns: one folder per node kind (packages/nodes/src/<kind>/) containing definition.ts, schema.ts, optionally geometry.ts / renderer.tsx / system.tsx / floorplan.ts / tool.tsx / move-tool.tsx / panel.tsx / parametrics.ts / preview.tsx. Exports builtinPlugin. Depends on editor, viewer, and core via their public surfaces — the same surfaces a third-party plugin uses (peer-dep style). Nothing in core/, viewer/, or editor/ may import from @pascal-app/nodes. The dependency arrow is one-way: framework code consults nodeRegistry, never reaches into a specific kind's folder.

Triggers that mean "this is probably in the wrong package"

1. Would the read-only /viewer/[id] route need this? If no, it belongs in apps/editor / packages/editor.
2. Does the name contain an editor-specific word? (Floorplan, Paint…, Draft…, Marquee, CursorBadge, HoverMode, …Tool, Moving…, Curving….) Default to editor and justify loudly if it's anywhere else.
3. Does the type or field reference a tool/mode/phase vocabulary? ('delete', 'paint-ready', 'material-paint', 'site'/'structure'/'furnish', 'build'/'edit'.) Belongs in useEditor, not useViewer or core.
4. Does the helper compute something only a 2D editor view needs? (Floorplan transforms, measurement offsets, SVG path builders, marquee bounds scoped to floorplan.) Editor. Generic 2D geometry that any view could use (polygon math, rotation, clamping, line thickening) can live in core as long as its names are generic — no Floorplan prefix.
5. Does a new store field have a setter that no part of the target layer ever calls? (e.g. setMaterialPreview in useViewer that only the editor would ever invoke.) That's a layering smell — the state belongs in the caller's layer.
6. Does the new file mention a specific kind by name? (door-…, wall-…, item-…, etc.) Then it belongs in packages/nodes/src/<kind>/, not under packages/viewer/src/components/renderers/<kind>/, packages/viewer/src/systems/<kind>.ts, packages/editor/src/components/tools/<kind>/, or packages/editor/src/components/ui/panels/<kind>-panel.tsx. Those legacy locations were deleted at Phase 6 cleanup — reintroducing one is a regression to the dispatch model.
7. Does an import line read from '@pascal-app/nodes' inside core/, viewer/, or editor/? Blocker. The Biome noRestrictedImports overrides in biome.jsonc already ban this, along with the rest of the arrow (core → three/R3F/viewer/editor, viewer → editor); if one slipped through, a framework package is reaching down into the plugin or up the arrow.

Write the classification down before writing findings. If core gains "Floorplan" types, the viewer gains paint-mode vocabulary, a renderer grows editor awareness, or a kind-specific file appears outside packages/nodes/src/<kind>/ — those are the blockers to lead with, not downstream symptoms.

4. Review checklist

A. Package boundaries

• packages/viewer/** does not import from @pascal-app/editor, apps/editor, or @pascal-app/nodes, and does not reference useEditor, tool state, phase, or mode.
• packages/core/** does not import Three.js, react-three-fiber, @pascal-app/viewer, @pascal-app/editor, or @pascal-app/nodes.
• packages/editor/** does not import from @pascal-app/nodes.
• packages/core/** does not introduce types or helpers named after an editor view (Floorplan*, Paint*, Draft*). Generic plan-geometry helpers are fine; view-specific vocabulary is not.
• No new case '<kind>': clauses (or equivalent kind-specific branching keyed on node.type) inside packages/viewer/** or packages/editor/**. Phase 6 deleted these; the dispatch happens via nodeRegistry. The exceptions left in tree are treeNodeByType (a lookup map, not a switch) and unit-formatting switches (centimeters / feet / inches). Any new case 'door'|'wall'|'item'… in a framework package is a blocker — the behavior belongs on the kind's NodeDefinition.
• Tools mutate useScene (committed state) and useLiveTransforms (ephemeral drag state); direct sceneRegistry mesh transforms are allowed only under the live-drag exception in wiki/architecture/tools.md. No business logic, no imports from packages/viewer.

B. Node registry & composition (packages/nodes)

If the PR adds or modifies a node kind, check against wiki/architecture/node-definitions.md and wiki/architecture/plugin-authoring.md:

• 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.