docs(arch): codify the unified snapping/modifier convention + enforce in review

The snapping model (Shift = cycle mode, Alt = force/free, mode-driven reads via
isGridSnapActive/isMagneticSnapActive/isAngleSnapActive, snapProfile-declared
context) lived only in code and the plan; tools.md still preached the legacy
"Shift = bypass snapping". Close the drift so the architecture review refuses
tool changes that revert to the old pattern:

- tools.md: replace the held-Shift-bypass manipulation policy with the unified
  mode-driven model + the single snap read path.
- interaction-scope.md: new "Snapping mode & modifiers" section (contexts, read
  path, modifiers, the chip-needs-a-scope rule) + a Rules bullet + the
  known-legacy MEP movers (migrate-on-touch) incl. the dual-path constraint
  (a bespoke mover must not open a `moving` scope — it re-mounts the generic
  mover via useMovingNode).
- review-architecture skill: add interaction-scope.md to the reads and a new
  "F. Interaction scope, snapping & modifiers" checklist — new shiftKey-bypass,
  ungated grid step, missing snapProfile, a new useEditor interaction flag, or a
  bespoke mover opening a moving scope are blockers; touching the legacy MEP
  movers forces migration.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Wassim SAMAD
2026-06-26 09:28:14 -04:00
co-authored by Claude Opus 4.8
parent 1de1923e47
commit f0206dc6eb
3 changed files with 69 additions and 21 deletions
@@ -25,6 +25,7 @@ Required on every review. Read the remaining pages on demand when the diff touch
- `wiki/architecture/spatial-queries.md` - `wiki/architecture/spatial-queries.md`
- `wiki/architecture/node-schemas.md` - `wiki/architecture/node-schemas.md`
- `wiki/architecture/events.md` - `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 skim the live charter at `plans/editor-node-registry.md` (in the private-editor repo) — it owns the current contract and which kind sits at which migration stage. If anything in the diff looks like a new dispatch surface or registry concept, also skim the live charter at `plans/editor-node-registry.md` (in the private-editor repo) — it owns the current contract and which kind sits at which migration stage.
@@ -135,6 +136,17 @@ If the PR adds or modifies a node kind, check against `wiki/architecture/node-de
- New node types are added by creating one folder under `packages/nodes/src/<kind>/` and registering its definition in `builtinPlugin.nodes`. Adding to a hand-maintained list elsewhere is a sign the registry hasn't absorbed that surface yet — check `plans/editor-node-registry.md` § "Known un-shimmed hardcoded lists" before assuming it's a violation. - New node types are added by creating one folder under `packages/nodes/src/<kind>/` and registering its definition in `builtinPlugin.nodes`. Adding to a hand-maintained list elsewhere is a sign the registry hasn't absorbed that surface yet — check `plans/editor-node-registry.md` § "Known un-shimmed hardcoded lists" before assuming it's a violation.
- `AnyNode` is hand-maintained for now (full runtime derivation would lose static typing); `packages/nodes/src/index.test.ts` is the drift gate. If a PR adds a kind to `AnyNode` without adding it to `builtinPlugin.nodes` (or vice versa), the parity test catches it — but flag it in review too. - `AnyNode` is hand-maintained for now (full runtime derivation would lose static typing); `packages/nodes/src/index.test.ts` is the drift gate. If a PR adds a kind to `AnyNode` without adding it to `builtinPlugin.nodes` (or vice versa), the parity test catches it — but flag it in review too.
### F. Interaction scope, snapping & modifiers
Apply when the diff touches a tool, a `move-tool` / `selection` / endpoint / reshape file, `lib/interaction/**`, `lib/snapping-mode.ts`, or `use-interaction-scope`. Source of truth: `wiki/architecture/interaction-scope.md` and `wiki/architecture/tools.md`.
- **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").
- **Known-legacy exceptions (migrate on touch).** The MEP move/endpoint tools (`packages/nodes/src/{duct-segment,pipe-segment,liquid-line,lineset,duct-fitting}/{move-tool,selection}.tsx`) still carry the legacy `shiftKey` bypass; they are tracked debt in `plans/editor-placement-interaction-overhaul.md`. A PR that **touches** one of these must migrate it onto the model above, not extend the legacy path. A PR that adds a **new** tool on the legacy `shiftKey`-bypass pattern is a blocker regardless.
## 5. Output format ## 5. Output format
Group findings by severity: Group findings by severity:
+33
View File
@@ -115,6 +115,38 @@ The policy is binary (`IDLE_POLICY` vs `ACTIVE_POLICY`) keyed on `isActive`.
--- ---
## Snapping mode & modifiers (the unified model)
Snapping is a persistent, **per-context**, always-visible mode — not a held-Shift bypass.
The active scope selects the *context*; the context's current mode selects the *behaviour*.
There is no per-kind snapping switch.
- **Contexts** (`lib/snapping-mode.ts`, `SNAP_PROFILES`): `wall` (grid/lines/angles/off, default grid),
`item` (lines/grid/off, default lines), `polygon` (grid/lines/off, default grid). A kind opts in by
declaring `NodeDefinition.snapProfile` (`'item' | 'structural'`); `snapContextOf(scope × profile)` maps
it — `structural` while **setting direction** (drafting / endpoint drag) → `wall` (angle-bearing),
`structural` otherwise (translate / curve) → `polygon` (no angle), `item``item`. No profile → no chip.
- **Single read path.** Tools read `isGridSnapActive()` / `isMagneticSnapActive()` / `isAngleSnapActive()`
(`store/use-editor`); the grid step is `useEditor.getState().gridSnapStep` gated on `isGridSnapActive()`.
These resolve the mode from the scope via `getActiveSnapContext()``snappingModeByContext[context]`.
- **Modifiers.** Shift (tap) cycles the mode for the active context; Ctrl (tap) cycles the grid step;
Alt (hold) is force / free (raw cursor + commit past invalid; for MEP runs, the vertical-riser carve-out).
Shift is **not** a snap bypass and Alt is **not** a toggle.
- **The chip is the scope's.** The contextual HUD shows the active context's mode and is the only place the
mode is cycled — so a tool that wants its chip must run inside a scope whose `snapContextOf` resolves
(a build tool, `drafting`, `placing`/`moving`, or `reshaping`).
**Known-legacy (migrate on touch).** A few bespoke movers predate this model and still read
`event.shiftKey` as a snap bypass with hardcoded steps: the MEP move/endpoint tools
(`packages/nodes/src/{duct-segment,pipe-segment,liquid-line,lineset,duct-fitting}/{move-tool,selection}.tsx`),
tracked in `plans/editor-placement-interaction-overhaul.md`. A PR that **touches** one must migrate it to
the model above, not extend the legacy path. Note: opening a `moving` scope from a bespoke mover is **not**
the migration — `useMovingNode()` reads the scope, so `tool-manager` re-mounts the generic
`MoveRegistryNodeTool` alongside it (the dual-path FPS/teleport bug). Resolve the mode without a global
`moving`/`reshaping` scope; see the plan's dual-path note.
---
## Migration status (strangler fig) ## Migration status (strangler fig)
The scope is the target source of truth, but the legacy `useEditor` flags still The scope is the target source of truth, but the legacy `useEditor` flags still
@@ -140,3 +172,4 @@ independent flag clear can't stomp an unrelated scope) to keep the scope in sync
- **`update` cannot change `kind`.** Switching interactions is a `begin`, not a patch. - **`update` cannot change `kind`.** Switching interactions is a `begin`, not a patch.
- **Hot-set and overlay policy are pure derivations of the scope** (and, for the hot-set, the candidate metadata). Don't branch overlay/picking behaviour on legacy flags — branch on the scope. - **Hot-set and overlay policy are pure derivations of the scope** (and, for the hot-set, the candidate metadata). Don't branch overlay/picking behaviour on legacy flags — branch on the scope.
- **Don't add new `useEditor` interaction flags.** New interactions go through the scope. - **Don't add new `useEditor` interaction flags.** New interactions go through the scope.
- **Snapping is mode-driven.** Read snap state through `isGridSnapActive` / `isMagneticSnapActive` / `isAngleSnapActive` (gate any grid step on the first); never bypass snapping via `event.shiftKey` / `modifiers.shiftKey`, and never hardcode an ungated grid step. Snappable kinds declare `snapProfile`. Shift cycles the mode; Alt is force/free.
+24 -21
View File
@@ -68,27 +68,30 @@ export function MyTool() {
- The offset must be cleared on tool unmount, cancel, *and* commit — both `mesh.position.set(0, 0, 0)` and `useLiveTransforms.clear(id)`. - The offset must be cleared on tool unmount, cancel, *and* commit — both `mesh.position.set(0, 0, 0)` and `useLiveTransforms.clear(id)`.
- The tool must not generate or mutate geometry in this path — only transform writes. Geometry generation still belongs in a core system. - The tool must not generate or mutate geometry in this path — only transform writes. Geometry generation still belongs in a core system.
- **No business logic in tools** — delegate geometry/constraint rules to core systems. - **No business logic in tools** — delegate geometry/constraint rules to core systems.
- **Guided manipulation is the default.** Placement, move, rotate, resize, endpoint drag, - **Snapping is mode-driven, not a held-Shift bypass.** Placement, move, rotate, resize,
and handle drag should behave as guided building mode: they help the user build quickly endpoint drag, and handle drag are guided building — grid/object snapping, canonical angle
with fewer mistakes through grid/object snapping, canonical angle increments, increments, alignment guides, distance feedback — but the active behaviour is an explicit,
alignment guides, and distance feedback. Holding Shift is the standard live bypass for always-visible, **per-context** mode (the contextual HUD chip), not a hidden held key:
those constraints: while Shift is held, tools should commit the raw pointer/angle - **Shift (tap)** cycles the snapping mode for the active context (`wall` grid/lines/angles/off ·
proposal instead of applying sticky snap or angle corrections. Passive measurement `item` lines/grid/off · `polygon` grid/lines/off — one persisted mode per context).
guides may remain visible only when they do not alter the proposal. If an interaction - **Alt (hold)** is force / free: commit the raw cursor past snap *and* past an invalid /
cannot use Shift because of an established shortcut or topology rule, document the colliding drop. It is the only momentary "bypass" key (plus the vertical-riser carve-out for MEP runs).
opt-out in its manipulation policy and explain the replacement behavior. - **Ctrl (tap)** cycles the grid step.
- **Constraints and guides can be decoupled.** When a stronger constraint owns the - Read snapping through the single path — `isGridSnapActive()` / `isMagneticSnapActive()` /
proposal, such as a wall segment's 15° angle lock, the tool may still publish passive `isAngleSnapActive()` (`store/use-editor`), which resolve the active mode from the interaction
dashed alignment/proximity guides as long as it does not apply the guide snap delta. scope via `getActiveSnapContext()`. **Never** read `event.shiftKey` / `event.nativeEvent.shiftKey` /
Use this for chained wall segments: users keep the fast constrained draft, but still see `modifiers.shiftKey` to bypass snapping, and never apply a grid step that isn't gated on
proximity feedback for later points. Shift remains the hard bypass for both correction `isGridSnapActive()` (`const step = isGridSnapActive() ? gridSnapStep : 0`). A snappable kind declares
and guide feedback. `NodeDefinition.snapProfile` (`'item' | 'structural'`) so its context, mode-set, and chip fall out
- **Help must mirror manipulation policy.** The shortcut dialog and floating helper panel with no per-kind switch. See [interaction-scope](interaction-scope.md) § "Snapping mode & modifiers"
are part of the interaction contract. Static shortcut docs should describe guided and `lib/snapping-mode.ts`.
building as the default and Shift as the live bypass. Floating help should be contextual - **Constraints and guides can be decoupled.** When a stronger constraint owns the proposal —
when enough state exists: Select mode can derive direct move, direct rotate, a wall segment's 45° lock while in `angles` mode — the tool may still publish passive dashed
multi-select, and Shift-bypass tips from the selected nodes and active modifiers; active alignment/proximity guides as long as it does not apply the guide snap delta. Use this for chained
tools can highlight the Shift bypass row while the modifier is held. wall segments: users keep the fast constrained draft but still see proximity feedback for later points.
- **Help mirrors the model.** The shortcut dialog and the contextual HUD are part of the interaction
contract: they describe the always-visible mode chip + `Alt` = force, **not** a hidden Shift bypass.
The HUD is driven by the active interaction scope, so it shows only the current context's controls.
- **Preview geometry is local** — transient meshes shown while a tool is active live in the tool component, not in the scene store. - **Preview geometry is local** — transient meshes shown while a tool is active live in the tool component, not in the scene store.
- **Clean up on unmount** — remove any pending/incomplete nodes *and* any live transforms/mesh offsets when the tool unmounts. - **Clean up on unmount** — remove any pending/incomplete nodes *and* any live transforms/mesh offsets when the tool unmounts.
- **Tools must not import from `@pascal-app/viewer`** — use the scene store and core hooks only. `sceneRegistry` is exported from `@pascal-app/core` and is the allowed door into the Three.js graph for the narrow purposes above. - **Tools must not import from `@pascal-app/viewer`** — use the scene store and core hooks only. `sceneRegistry` is exported from `@pascal-app/core` and is the allowed door into the Three.js graph for the narrow purposes above.