From 6b0fe5b99c1d3eb32887dbbefdd39594b01bdb82 Mon Sep 17 00:00:00 2001 From: Wassim SAMAD Date: Wed, 27 May 2026 08:02:08 -0400 Subject: [PATCH] review-architecture: cover new registry capabilities (#338) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add bullets to §4.B for surfaces that landed after the 2026-05-20 skill update: - def.capabilities.paint (PaintCapability dispatch for paint mode) - def.capabilities.floorPlaced (generic FloorElevationSystem) - def.surfaceRole (Solid/Rendered/Clay render-mode pipeline) Also add a verb-not-host-kind rule and mark capabilities.roofAccessory as documented tech debt — extending the *Accessory / *Hosted shape is now a blocker rather than a precedent. Co-authored-by: Claude Opus 4.7 (1M context) --- .agents/skills/review-architecture/SKILL.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.agents/skills/review-architecture/SKILL.md b/.agents/skills/review-architecture/SKILL.md index 3ffd9b43..3e25a8f6 100644 --- a/.agents/skills/review-architecture/SKILL.md +++ b/.agents/skills/review-architecture/SKILL.md @@ -99,6 +99,10 @@ If the PR adds or modifies a node kind, check against `wiki/architecture/node-de - **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`. - **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. The bug history is documented in `plans/editor-node-registry.md` ("Capability-driven move dispatch"). +- **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 === '')` 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`). +- **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 `` 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 === '')` 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` / `cuttable` / `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"). 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//*`, `viewer/src/systems/-system.tsx`, `editor/src/components/tools//*`, `editor/src/components/ui/panels/-panel.tsx`, `editor/src/components/ui/helpers/-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 ``. 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.