From 76089d85ea494cf17fbe0720cc282ba1ae8d88ea Mon Sep 17 00:00:00 2001 From: Wassim SAMAD Date: Fri, 26 Jun 2026 11:58:11 -0400 Subject: [PATCH] =?UTF-8?q?docs(arch):=20record=20wall/fence=20Alt?= =?UTF-8?q?=E2=86=92chain-toggle=20+=20altKey-alignment=20known-legacy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit interaction-scope.md + review-architecture skill: the sanctioned Alt-as-toggle (wall/fence chain mode, the one place Alt-as-force is meaningless), and a second known-legacy pattern — `event.altKey` alignment-bypass in the roof/polygon/slab previews + ceiling/slab snap paths (migrate-on-touch; wall+fence already done). Co-Authored-By: Claude Opus 4.8 --- .agents/skills/review-architecture/SKILL.md | 3 ++- wiki/architecture/interaction-scope.md | 28 ++++++++++++++------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/.agents/skills/review-architecture/SKILL.md b/.agents/skills/review-architecture/SKILL.md index 30774c30..56cb4821 100644 --- a/.agents/skills/review-architecture/SKILL.md +++ b/.agents/skills/review-architecture/SKILL.md @@ -145,7 +145,8 @@ Apply when the diff touches a tool, a `move-tool` / `selection` / endpoint / res - **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. +- **`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 `plans/editor-placement-interaction-overhaul.md`; 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). ## 5. Output format diff --git a/wiki/architecture/interaction-scope.md b/wiki/architecture/interaction-scope.md index c1e88f15..8a07f72c 100644 --- a/wiki/architecture/interaction-scope.md +++ b/wiki/architecture/interaction-scope.md @@ -131,19 +131,29 @@ There is no per-kind snapping switch. 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. + Shift is **not** a snap bypass. Alt is **not** a snap toggle. **Exception — wall/fence drafting:** those + tools have no force role (a wall/fence always places), so a clean **Alt-tap** cycles the chain mode + (`wallChainMode` room/single, `fenceChainMode` continuous/single) — wired in `hooks/use-keyboard.ts` via + `isChainModeContext()`, persisted in `useEditor`, surfaced as a clickable HUD chip. This is the one + sanctioned Alt-as-toggle, and only where Alt-as-force is meaningless. - **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. +**Known-legacy (migrate on touch).** Two legacy modifier patterns predate this model and survive in +spots not yet touched; both are 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: +1. **`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`). + 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. +2. **`event.altKey` as an alignment bypass** — the roof / polygon / slab pointer-move previews in + `components/editor/floorplan-panel.tsx` and the ceiling/slab `resolveSlabPlanPointSnap` / `resolveCeilingPlanPointSnap` + paths still pass `event.altKey` to suppress Figma-alignment. Alignment must instead follow the magnetic snap + mode (`bypass: !isMagneticSnapActive()`). **Already migrated (do not regress):** wall + fence drafting (3D + `{wall,fence}/tool.tsx` + the 2D `use-floorplan-background-placement.ts` / `floorplan-panel.tsx` paths), where + Alt was freed for the chain-mode toggle above. ---