From f0206dc6eb41649167e3efde781ad504a2a29e78 Mon Sep 17 00:00:00 2001 From: Wassim SAMAD Date: Fri, 26 Jun 2026 09:28:14 -0400 Subject: [PATCH] docs(arch): codify the unified snapping/modifier convention + enforce in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .agents/skills/review-architecture/SKILL.md | 12 ++++++ wiki/architecture/interaction-scope.md | 33 +++++++++++++++ wiki/architecture/tools.md | 45 +++++++++++---------- 3 files changed, 69 insertions(+), 21 deletions(-) diff --git a/.agents/skills/review-architecture/SKILL.md b/.agents/skills/review-architecture/SKILL.md index 88e952d3..30774c30 100644 --- a/.agents/skills/review-architecture/SKILL.md +++ b/.agents/skills/review-architecture/SKILL.md @@ -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/node-schemas.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. @@ -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//` 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. +### 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//{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 Group findings by severity: diff --git a/wiki/architecture/interaction-scope.md b/wiki/architecture/interaction-scope.md index 05186e67..c1e88f15 100644 --- a/wiki/architecture/interaction-scope.md +++ b/wiki/architecture/interaction-scope.md @@ -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) 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. - **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. +- **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. diff --git a/wiki/architecture/tools.md b/wiki/architecture/tools.md index 2ab42b2d..90328c7c 100644 --- a/wiki/architecture/tools.md +++ b/wiki/architecture/tools.md @@ -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 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. -- **Guided manipulation is the default.** Placement, move, rotate, resize, endpoint drag, - and handle drag should behave as guided building mode: they help the user build quickly - with fewer mistakes through grid/object snapping, canonical angle increments, - alignment guides, and distance feedback. Holding Shift is the standard live bypass for - those constraints: while Shift is held, tools should commit the raw pointer/angle - proposal instead of applying sticky snap or angle corrections. Passive measurement - guides may remain visible only when they do not alter the proposal. If an interaction - cannot use Shift because of an established shortcut or topology rule, document the - opt-out in its manipulation policy and explain the replacement behavior. -- **Constraints and guides can be decoupled.** When a stronger constraint owns the - proposal, such as a wall segment's 15° angle lock, the tool may still publish passive - dashed alignment/proximity guides as long as it does not apply the guide snap delta. - Use this for chained wall segments: users keep the fast constrained draft, but still see - proximity feedback for later points. Shift remains the hard bypass for both correction - and guide feedback. -- **Help must mirror manipulation policy.** The shortcut dialog and floating helper panel - are part of the interaction contract. Static shortcut docs should describe guided - building as the default and Shift as the live bypass. Floating help should be contextual - when enough state exists: Select mode can derive direct move, direct rotate, - multi-select, and Shift-bypass tips from the selected nodes and active modifiers; active - tools can highlight the Shift bypass row while the modifier is held. +- **Snapping is mode-driven, not a held-Shift bypass.** Placement, move, rotate, resize, + endpoint drag, and handle drag are guided building — grid/object snapping, canonical angle + increments, alignment guides, distance feedback — but the active behaviour is an explicit, + always-visible, **per-context** mode (the contextual HUD chip), not a hidden held key: + - **Shift (tap)** cycles the snapping mode for the active context (`wall` grid/lines/angles/off · + `item` lines/grid/off · `polygon` grid/lines/off — one persisted mode per context). + - **Alt (hold)** is force / free: commit the raw cursor past snap *and* past an invalid / + colliding drop. It is the only momentary "bypass" key (plus the vertical-riser carve-out for MEP runs). + - **Ctrl (tap)** cycles the grid step. + - Read snapping through the single path — `isGridSnapActive()` / `isMagneticSnapActive()` / + `isAngleSnapActive()` (`store/use-editor`), which resolve the active mode from the interaction + scope via `getActiveSnapContext()`. **Never** read `event.shiftKey` / `event.nativeEvent.shiftKey` / + `modifiers.shiftKey` to bypass snapping, and never apply a grid step that isn't gated on + `isGridSnapActive()` (`const step = isGridSnapActive() ? gridSnapStep : 0`). A snappable kind declares + `NodeDefinition.snapProfile` (`'item' | 'structural'`) so its context, mode-set, and chip fall out + with no per-kind switch. See [interaction-scope](interaction-scope.md) § "Snapping mode & modifiers" + and `lib/snapping-mode.ts`. +- **Constraints and guides can be decoupled.** When a stronger constraint owns the proposal — + a wall segment's 45° lock while in `angles` mode — the tool may still publish passive dashed + alignment/proximity guides as long as it does not apply the guide snap delta. Use this for chained + 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. - **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.