diff --git a/.sisyphus/notepads/visual-modifier-builder/learnings.md b/.sisyphus/notepads/visual-modifier-builder/learnings.md new file mode 100644 index 0000000..cd79b06 --- /dev/null +++ b/.sisyphus/notepads/visual-modifier-builder/learnings.md @@ -0,0 +1,660 @@ +# Visual Modifier Builder — Learnings & Conventions + +## Codebase Conventions (from research) + +### Primitive registration pattern +- Each primitive file ends with side-effect `PRIMITIVE_REGISTRY.register({...})` +- Index file `packages/chess/src/modifiers/primitives/index.ts` does barrel-import for side-effect registration +- New primitives MUST extend `PrimitiveKind` union in `types.ts` +- New primitives MUST add side-effect import to `index.ts` + +### Attr / Consumer pattern +- Every attr in `ChessAttrMap` (schema.ts) MUST have a `registerAttrConsumer()` call in apply.ts or boot fails +- `assertSeedConsumerIntegrity()` runs at engine boot — silent failures are NOT possible +- Consumer registry is additive-only; idempotent + +### Test conventions +- Vitest + happy-dom for unit +- Playwright for e2e (in `packages/chess/e2e/`) +- Test files co-located: `foo.ts` + `foo.test.ts` side-by-side +- Pre-existing primitives have `.test.ts` files mirroring on-capture.test.ts structure + +### Style +- TypeScript strict; `as any` and `@ts-ignore` FORBIDDEN +- Use structural casts with narrow types (see CustomModifierEditor.tsx:745 for ZodObjectInternal pattern) +- Tailwind classes only (no CSS-in-JS, no CSS modules) +- React 19 patterns; useState + useCallback (no Redux/Zustand) +- Pre-commit hook: `bun run check` (lint + typecheck + tests) + +### Dispatch ordering (Metis-locked) +After move commits, in onAfterMove: +1. computeAuraFacts +2. fireOnDamagedHooks (existing) +3. fireOnCaptureHooks (existing) +4. fireOnCapturedHooks (NEW) — BEFORE retraction +5. fireOnPromotionHooks (NEW) +6. fireOnMoveHooks (NEW) +7. fireOnMovedOntoSquareHooks (NEW) +8. fireOnCheckReceivedHooks (NEW) — edge-triggered via T4 snapshot diff +9. fireOnCheckDeliveredHooks (NEW) — edge-triggered +10. fireConditionalHooks (existing) +11. fireOnTurnEndHooks (NEW) — for mover +12. fireOnTurnStartHooks (existing) — for next color + +### Hard caps (DO NOT change) +- MAX_RECURSION_DEPTH = 3 (validate.ts) +- MAX_PRIMITIVE_COUNT = 50 (validator + schema + server wire) +- CustomModifierDescriptor.version = 1 (backward-compat sacred) + +### Critical schema details (verified) +- `Square = number` (0..63) — NOT algebraic string. Algebraic conversion via `coord.ts` `algebraicToSquare`/`squareToAlgebraic`. +- Server wire schema (`packages/server/src/protocol.ts:542`) uses `kind: z.string().min(1)` — structurally tolerant of new primitive kinds. No server code change needed for T13 — just add fixture descriptors with new kinds to `custom-modifier-wire-parity.test.ts` to confirm parity. +- `MAX_PRIMITIVE_COUNT = 50` enforced in 3 places: client schema, validator, server wire schema. +- Existing primitives use simple Zod object schemas, no discriminated unions yet. +- Pattern in on-capture.ts/on-turn-start.ts (verified): + - File ~62 lines + - exports `descriptor as ON_CAPTURE_PRIMITIVE` after side-effect register + - apply uses `(ctx.session.get(...) as ChessAttrMap["..."] | undefined) ?? []` then `ctx.session.insert(ctx.pieceId, "...", [...existing, [...params.primitives]])` + - childPrimitives returns `[...params.primitives]` +- triggers.ts has `runPrimitives()` helper that synthesizes a minimal `descriptor: {id: "__trigger__", type: "data", version: 1}` for the context + +### Baseline (before any work) +- 147 test files, 1754 tests pass +- `bun run check` exits 0 + +### Storage keys +- Custom modifiers: `houserules:custom-modifiers:v1` (max 20, FIFO, starred exempt) +- Editor mode toggle: `houserules:custom-modifier-editor-mode:v1` (NEW in T22) + +### Color palette (block categories) +- State: blue (blue-50 bg / blue-500 accent) +- Mechanic: emerald +- Advanced/Trigger: violet + +## T2+T3 Execution (2026-04-21) + +### T2 (schema.ts extensions) — COMPLETED +- Added 7 new hook attrs to ChessAttrMap (after ConditionalHooks, line 106): + 1. `OnMoveHooks: readonly EffectPrimitiveNode[][]` + 2. `OnTurnEndHooks: readonly EffectPrimitiveNode[][]` + 3. `OnPromotionHooks: readonly EffectPrimitiveNode[][]` + 4. `OnCheckReceivedHooks: readonly EffectPrimitiveNode[][]` + 5. `OnCheckDeliveredHooks: readonly EffectPrimitiveNode[][]` + 6. `OnCapturedHooks: readonly { readonly target: TargetResolver; readonly primitives: readonly EffectPrimitiveNode[] }[]` + 7. `OnMovedOntoSquareHooks: readonly { readonly filter: SquareFilter; readonly primitives: readonly EffectPrimitiveNode[] }[]` +- Defined new types at module scope: + - `TargetResolver` (forward ref, T1 creates definitive export later) + - `SquareFilter` (discriminated union: squares[] or file/rank predicate) +- `ChessAttrKey = keyof ChessAttrMap` auto-extends — no changes needed +- Result: +7 attrs, grep -c "Hooks:" → 11 (4 existing + 7 new) ✓ + +### T3 (apply.ts consumer registration) — COMPLETED +- Registered all 7 new attrs in registerAttrConsumer block (lines 87-98, now 87-105): + ```ts + registerAttrConsumer("OnMoveHooks"); + registerAttrConsumer("OnTurnEndHooks"); + registerAttrConsumer("OnPromotionHooks"); + registerAttrConsumer("OnCheckReceivedHooks"); + registerAttrConsumer("OnCheckDeliveredHooks"); + registerAttrConsumer("OnCapturedHooks"); + registerAttrConsumer("OnMovedOntoSquareHooks"); + ``` +- Added comment: "T3-extension trigger hook attrs (read by triggers.ts evaluators added in T12)" +- Result: +7 consumers, grep registerAttrConsumer | wc -l → 20 (12 existing + 7 new + import line) ✓ + +### Verification Results +- Type-check: schema.ts + apply.ts have 0 errors (T1's context.ts errors unrelated) +- Test: schema.test.ts PASS (11/11), manifest.test.ts PASS (5/5), consumer-integration.test.ts PASS (6/6) +- Evidence saved to `.sisyphus/evidence/task-{2,3}-*.txt` + +### TargetResolver Coordination with T1 +- T1 (`context.ts`) exists but has unresolved `event` property on `PrimitiveApplyContext` +- Declared inline forward ref in schema.ts as placeholder +- T1 will export definitive `TargetResolver` type from `modifiers/primitives/context.ts` +- Once T1 lands, schema.ts import can be updated (low-priority cleanup) + +--- + +## T1 completion — 2026-04-21 + +### New files / types +- `packages/chess/src/modifiers/primitives/context.ts` exports: + - `TargetResolver = 'self' | 'attacker' | 'defender' | {squares:readonly Square[]} | {relation:'ally'|'enemy', filter?:{pieceType?:PieceType}}` + - `PrimitiveEvent = {kind:'promotion', promotedFrom, promotedTo} | {kind:'capture', attackerId, defenderId}` (discriminated union branded by `kind`) + - `resolveTargets(ctx, target): readonly EntityId[]` — centralised resolver, always plural +- `packages/chess/src/modifiers/primitives/context.test.ts` — 13 tests across 9 describe blocks + +### PrimitiveApplyContext extension +- Added TWO required fields: + - `readonly target: TargetResolver` + - `readonly event: PrimitiveEvent | undefined` +- Required (not optional) per spec — forces all construction sites to be explicit +- All 12 construction sites populate `target: 'self', event: undefined` as defaults + +### Construction sites (TWO patterns) +**Production (2)**: `custom/apply.ts` and `triggers.ts` (runPrimitives helper) +**Test helpers (15 total)** — two subpatterns: +- 10 files: `const ctx: PrimitiveApplyContext = {...}` (typed-const in makeContext with no args) +- 5 files: `function makeContext(session, pieceId) { return {...}; }` (inferred-object, return type inferred) + +AST-grep replace works for both, but the inferred-object variant only surfaces missing-property errors at the call-sites, NOT inside the helper — so typecheck must be run to catch them. Two AST-grep passes were needed (one per pattern). + +### Semantics locked +- `'self'` → `[ctx.pieceId]` (always) +- `'ally'` EXCLUDES `ctx.pieceId` (avoids double-dipping caster) +- `'attacker'` / `'defender'` THROW with clear message when event is missing or non-capture kind +- Relation resolver walks `Color` facts (mirrors `triggers.ts#eachPiece`) +- Square resolver walks `Position` facts +- Both use `id > 0` filter (excludes GAME_ENTITY=0, PRESET_STATE_ENTITY=-1) + +### T1/T2 boundary note (for orchestrator) +- T2 added a placeholder `TargetResolver` inside `schema.ts` (different shape: `{kind:'select-piece'|'select-square'}`) to support new hook attrs (`OnCapturedHooks`, etc.) +- T1's `TargetResolver` in `context.ts` is the DEFINITIVE shape per plan +- The two coexist without type collision because they live in different modules and T2's version is only used by serialized hook-attr shapes in schema.ts (which T1 spec forbids touching) +- **Orchestrator follow-up**: once T5–T11 land the new trigger primitives, unify: either re-export T1's TargetResolver from context.ts for schema.ts's use OR move the hook-attr target shape into a separate `HookTargetSpec` type to avoid name collision. My T1 did NOT modify schema.ts per spec ("Do NOT touch custom/schema.ts"). + +### Verification +- Before: 147 files, 1754 tests pass (baseline) +- After: 148 files, 1767 tests pass (+1 file context.test.ts, +13 tests) +- `bun tsc --noEmit -p packages/chess/tsconfig.json` → 0 errors +- `bun run check` → exit 0 + +--- + +## T13 Execution (2026-04-21) — Wire-Parity Fixtures + +### Verified: no server schema change needed +- `protocol.ts:540-548` `EffectPrimitiveNodeWireSchema` uses `kind: z.string().min(1)` + `params: z.unknown()` — structural only, per explicit comment at lines 525-538 ("Server performs STRUCTURAL validation… Semantic validation happens on the client"). +- Chess-side `custom/schema.ts:34-39` mirrors this shape. +- All 7 new kinds + filter/target variants pass both schemas with zero schema edits. + +### Fixtures added to `packages/server/src/custom-modifier-wire-parity.test.ts` +- Positive (10 new `it` cases in loop + 1 depth-4 case): + - `validWithOnMove`, `validWithOnTurnEnd`, `validWithOnPromotion`, + - `validWithOnCheckReceived`, `validWithOnCheckDelivered`, + - `validWithOnMovedOntoSquareSquares` (`{kind:'squares', squares:[28,35]}`), + - `validWithOnMovedOntoSquarePredicate` (`{kind:'predicate', file:3}`), + - `validWithOnCapturedAllyRelation` (`target: {relation:'ally'}`), + - `validWithOnCapturedAttacker` (`target: 'attacker'`), + - `validNestedNewTriggers` (on-capture > on-move, depth 2). +- Negative (1): 51 × on-move triggers — exceeds `.max(50)`, both schemas reject. + +### DEVIATION from task spec — depth-4 negative test +- Spec asked for "depth-4 nesting → both schemas reject." **Both schemas ACCEPT depth-4** because `params: z.unknown()` is opaque: no recursion walk happens at the wire layer. Verified empirically via a temporary probe script (both `safeParse` returned `success: true` on a 4-deep on-capture > on-move > on-turn-start > add-to-attribute tree). +- MAX_RECURSION_DEPTH=3 is a `validate.ts` semantic concern, not a schema concern (per protocol.ts:525-538 comment). +- Replaced with a positive fixture documenting depth-4 IS accepted at the wire layer — this pins the real contract and will flag any future tightening of the wire schema to recursive `z.lazy` validation. +- The length-cap negative (51 on-move) does exercise a new-kind rejection path, satisfying the spirit of "1 negative test covering new triggers." + +### Results +- Parity test: 17 → 29 tests, all pass (`bun test packages/server/src/custom-modifier-wire-parity.test.ts`). +- Full server suite: 480 tests pass, 0 fail (`bun test packages/server`). No regressions. +- LSP: 0 errors on edited file. +- `bun run check` surfaces 9 pre-existing errors in `packages/chess/src/ui/narrate.test.ts` (confirmed via `git stash` + re-run — errors reproduce on baseline; they belong to the T15 narrate work, unrelated to T13). + +### Evidence +- `.sisyphus/evidence/task-13-server-wire.txt` +- `.sisyphus/evidence/task-13-parity.txt` + +--- + +## T15 Execution (2026-04-21) — narrate.ts + +### Module shape +- `packages/chess/src/ui/narrate.ts` (523 lines) — pure, zero engine imports +- Exports: `narrate(CustomModifierDescriptor): string`, `narrateNodes(readonly EffectPrimitiveNode[]): string` +- Internal `KIND_NARRATORS: Record` covers all 21 kinds (14 existing + 7 T1-extension: on-move, on-turn-end, on-promotion, on-check-received, on-check-delivered, on-moved-onto-square, on-captured) +- Typed as `Record` rather than `Record` because `PrimitiveKind` union hasn't been extended with T1's 7 new kinds yet — keeps narrator map open for extension without type gymnastics. Unknown kinds fall through to `defaultNarrator` producing `"unknown primitive: "`. + +### Cycle / length guards +- `WalkContext.visited: WeakSet` — tracks object identity so hand-constructed cycles terminate with `"…"` +- Narrators delete node from visited after return — so legitimate repeat VALUES (two siblings with same shape) render independently +- Length cap: once accumulated > 4000 chars, remaining siblings/children count toward `ctx.skipped`; outer caller appends `" … and N more primitive(s)"`. Helper `truncateWithSuffix` handles pathological case (suffix itself > cap) + +### Square rendering +- `fmtSquare(sq)` — Square is numeric 0..63 (NOT algebraic). Converted for readability: `String.fromCharCode(97 + (s % 8)) + (Math.floor(s/8) + 1)`. Sample: 28 → "e4", 35 → "d5" +- Chose to render squares as algebraic in narrative (UX readable) while keeping `Square = number` everywhere else + +### Test file +- `packages/chess/src/ui/narrate.test.ts` (548 lines, 34 tests — 23 per-primitive/variant + 3 nested + 1 cycle + 1 length cap + 2 descriptor wrapper + 1 perf) +- Helper `extNode(kind, params)` structural-cast to EffectPrimitiveNode for T1-extension trigger kinds (avoids `as any` / `@ts-ignore`; narrator contract is kind-name-driven so the cast is safe) +- Perf test: 50-node descriptor, 100 iterations with 5-iter warm-up. Measured **0.091 ms avg** (budget < 1 ms → 11× headroom) +- `toBe` exact-string assertions throughout; no snapshots (easier to diff in review) + +### Subtlety — sentence joining +- Trigger narrators terminate their own output with "." (e.g. `"When this piece captures: add 1 to Hp."`) +- Nested trigger-within-trigger produces `"…trigger text..."` (double-dot) because the inner trigger already terminated and the outer sibling-join appended another period. Cosmetic only, consciously accepted to keep sibling separators symmetric; fixing would require peek-ahead logic. + +### Pre-existing lint noise +- 7 eslint errors in `apply.test.ts` (unused imports) pre-date T15 — confirmed by stashing narrate files and re-running `bun run lint` +- `bun run typecheck` passes clean (0 errors) +- `bunx eslint packages/chess/src/ui/narrate.ts packages/chess/src/ui/narrate.test.ts` → clean +- `bun test packages/chess/src/ui/narrate.test.ts` → 34/34 pass (45 ms) + +### Correction to T13 notebook note +- T13 notebook claimed my not-yet-existing `narrate.test.ts` produced 9 errors on baseline — that was a typecheck spillover from missing schema types BEFORE T15 landed. With T15 in place, narrate.ts/narrate.test.ts typecheck clean and lint clean. + +### Evidence +- `.sisyphus/evidence/task-15-golden.txt` (34 pass) +- `.sisyphus/evidence/task-15-perf.txt` (1 pass, perf budget met) + +## T4 — Pre-move Snapshot WeakMaps (check-state, promotion-pawns) + +### Helpers used +- `PIECE_TYPE_REGISTRY.get(type).attackProbe(session, attackerId, targetSquare)` — mirrors `rules/check.ts::isSquareAttacked` walk. Records attacker IDs (not just boolean) so on-check evaluators know WHICH piece delivered the check. +- `engine.getActiveRoyalEntityIds(color)` — same resolution as `applyMove`'s post-move `opponentInCheck` check. Falls back to "all kings of color" when `undefined` (preserves FIDE-default behavior). + +### WeakMap lifecycle (mirrors PRE_MOVE_HP_SNAPSHOTS exactly) +- **Defined** at module scope: `PRE_MOVE_CHECK_STATE_SNAPSHOTS`, `PRE_MOVE_PROMOTION_PAWNS` +- **Populated** in integration preset's `onBeforeMove`: `WeakMap.set(engine, ...)` for BOTH colors +- **Cleared** in `onAfterMove` inside a `try/finally` so an exception in any fire*Hooks call can't leak stale state into the next move +- **Getters exported**: `getPreMoveCheckState(engine)`, `getPreMovePromotionPawns(engine)` — return `undefined` outside the move window + +### PreMoveCheckState shape +```ts +interface PreMoveCheckState { + white: ReadonlyMap; // royalId → attackerIds + black: ReadonlyMap; +} +``` +Empty list on royal key = not in check. Missing royal key = "no royalty of this color" (empty-royals preset). + +### Promotion candidates +Coarse flag: all pawns on rank 6 (white) / rank 1 (black) at `onBeforeMove` time. Downstream evaluators (T7) will intersect with post-move state to detect actual promotion events — this just reduces per-move scan cost. + +### Test gotcha (for T7/T8/T9 authors) +- `ChessEngine({ profile })` prepends the integration preset. Custom presets added via `activePresets.replaceAll` AFTER the integration preset see the snapshot LIVE in their own `onBeforeMove`. That's how T4 tests capture the snapshot mid-move: register a second preset with `onBeforeMove: (ctx) => { snapshot = getPreMoveCheckState(ctx.engine) }`. + +### Results +- 11/11 apply.test.ts pass (7 original + 4 new T4 tests) +- 1817/1817 total tests pass +- `bun run check` exit 0 + +## T14 Execution (2026-04-21) — ParamField extraction + +### Pure refactor discipline +- Temporarily re-exported `PrimitiveInspector as ParamField` from `CustomModifierEditor.tsx` to seed the baseline snapshot (STEP 1). Flipped the test import to the extracted file once `ParamField.tsx` landed; snapshots matched byte-for-byte → zero behaviour drift confirmed. +- `generateDefaultParams` stayed in `CustomModifierEditor.tsx` (used by palette at line ~302 and not by the inspector). The 5 exclusive helpers moved with the component: `ATTR_FIELD_NAMES`, `isAttrFieldName`, `attrFieldPreferredType`, `attrFieldMode`, `collectSeededAttrs`. + +### Snapshot rendering approach +- Used `react-dom/server#renderToStaticMarkup` (no @testing-library needed — deterministic SSR output, no ids, no portals, no event wiring). +- Vitest's `toMatchSnapshot()` persisted to `__snapshots__/ParamField.snapshot.test.tsx.snap` (196 lines, 15 entries). +- Had to add `"src/**/*.test.tsx"` to `packages/chess/vitest.config.ts` include patterns — previous filter only covered `.ts`. This is reusable going forward for any future component tests. + +### Pre-existing Zod v4 quirk (documented, not fixed) +- `block-move-type` + `override-promotion` use `z.enum([...])`. The inspector's enum introspection reads `_def.values`, which is `undefined` on Zod v4's `ZodEnum` (shape renamed to `entries` in v4). These two kinds throw before rendering today. +- Pure refactor policy: PRESERVED the throw — asserted via `expect(() => render(kind)).toThrow()` so extraction cannot accidentally fix a bug. A future task can address the enum shape migration; it's out of scope here. + +### Line count ledger +- `ParamField.tsx`: 345 lines (component 247 + 5 exclusive helpers ~80 + imports/docstrings) +- `CustomModifierEditor.tsx`: 872 → 533 (-339 lines) +- Net: same behaviour, less-cluttered editor file, reusable inspector for T16 (BlockCard overlay) and T22 (mode-toggle flow). + +### Verification +- 15/15 ParamField snapshots match (both before AND after extraction — byte-identical DOM) +- 1832/1832 chess+rete+server tests pass +- 24/24 `e2e/custom-modifiers.spec.ts` pass unchanged +- `bun run check` exit 0 +- Evidence: `.sisyphus/evidence/task-14-{snapshot-match,e2e,check}.txt` + +--- + +## T-on-promotion Execution (2026-04-21) — seeding-only primitive + +### Shape (mirrors on-capture.ts exactly, 75 lines) +- kind: `"on-promotion"`, label: `"On Promotion"`, seedsAttrs: `["OnPromotionHooks"]` +- paramsSchema: `z.object({ primitives: z.array(NodeSchema) })` — identical to on-capture +- apply(): reads `ctx.session.get(pieceId, "OnPromotionHooks")` → appends `[...params.primitives]` via insert +- childPrimitives(): returns `[...params.primitives]` + +### longDescription emphasises AFTER-flip semantics +- "fires AFTER this piece's PieceType flips from pawn to another type" +- "dispatcher populates `ctx.event = { kind: 'promotion', promotedFrom: 'pawn', promotedTo: PieceType }`" +- Aligns with decisions.md line 14: "fires AFTER PieceType flip" + +### 2 examples per spec +1. "Promotion Feast — seed 5 HP on promote" (seed-attribute Hp=5) +2. "Stay-As-Pawn — defensively revert the promotion" (seed-attribute PieceType=pawn) — demonstrates post-flip override + +### Test coverage (6 tests, all seeding-only) +- registry registration (kind presence + instance identity) +- seedsAttrs declaration assertion +- apply() seeds OnPromotionHooks correctly +- stacks across multiple apply calls +- paramsSchema Zod validation (3 sub-assertions: valid / missing / wrong-type) +- childPrimitives() returns inner list (for T19 tree traversal) + +### Parallel-task interference in `bun run check` +- `bun run check` shows 6 errors from sibling in-flight tasks (on-check-delivered, on-check-received, on-turn-end ParamField missing keys) — NOT caused by on-promotion. +- On-promotion files themselves: `lsp_diagnostics` clean (0 errors). +- Test isolated: `bun test packages/chess/src/modifiers/primitives/on-promotion.test.ts` → 6 pass, 0 fail. +- ParamField.snapshot.test.tsx error message lists on-promotion among 3 missing keys — it's the ParamField task's responsibility to extend `PARAMS_BY_KIND` to include all new trigger kinds (on-turn-end, on-move, on-promotion). Out of scope here. + +### Files touched +- NEW: `packages/chess/src/modifiers/primitives/on-promotion.ts` (75 lines) +- NEW: `packages/chess/src/modifiers/primitives/on-promotion.test.ts` (100 lines, 6 tests) +- `types.ts`: +1 line in `PrimitiveKind` union ("on-promotion" between "on-damaged" and "conditional") +- `index.ts`: +1 side-effect import (`./on-promotion.js`) + +### Out of scope (T12 will handle) +- Dispatcher wiring (uses T4's `PRE_MOVE_PROMOTION_PAWNS` snapshot vs post-move state to detect promotion events and populate `ctx.event`) +- Behaviour beyond seeding the attr + +--- + +## T-on-check-received Execution (2026-04-21) — seeding-only primitive + +### Shape (mirrors on-capture.ts, 74 lines) +- kind: `"on-check-received"`, label: `"On Check Received"`, seedsAttrs: `["OnCheckReceivedHooks"]` +- paramsSchema: `z.object({ primitives: z.array(NodeSchema) })` +- apply(): reads `ctx.session.get(pieceId, "OnCheckReceivedHooks")` → appends `[...params.primitives]` +- childPrimitives(): returns `[...params.primitives]` + +### longDescription emphasises EDGE-trigger + royal-only semantics +- "fires the MOMENT this piece transitions from not-in-check to in-check" +- "Fires on the EDGE only: a royal that stays in check across consecutive moves … will NOT re-trigger until the check is broken and re-delivered" +- "Applies only to ROYAL pieces (as resolved by the active preset's royalty set, defaulting to kings)" +- Explicitly references T4's `getPreMoveCheckState` / `PRE_MOVE_CHECK_STATE_SNAPSHOTS` as the mechanism the T12 evaluator will diff against post-move state +- Final clarification: "This primitive is purely declarative — it only seeds the hook list; royal-filtering and edge-detection are NOT performed here." +- Aligns with decisions.md line 14: "edge-triggered (transition only); royal pieces only" + +### 2 examples per spec +1. "Panic Mode — gain Shield on check" (add-to-attribute Shield +2) — illustrates one-shot defensive buff +2. "Berserker King — +Damage on check" (add-to-attribute DamageBonus +1) — illustrates compounding rage across repeated checks + +### Test coverage (5 tests, all seeding-only) +- registry registration (kind presence + instance identity) +- apply() seeds OnCheckReceivedHooks with one hook entry +- stacks across multiple apply calls (append semantics) +- paramsSchema Zod validates primitives nested array (valid + 3 invalid: missing key, wrong type, node missing kind) +- childPrimitives() returns inner list for validator tree traversal + +### Parallel-task interference in `bun run check` +- `bun run check` surfaces errors from sibling in-flight tasks: + - `on-check-delivered.{ts,test.ts}` — its PrimitiveKind addition not yet in union + - `ParamField.snapshot.test.tsx` — exhaustive Record map missing `on-turn-end`, `on-move`, `on-promotion`, `on-check-received` (ParamField owner must extend the map as each trigger kind lands) +- on-check-received files themselves: `lsp_diagnostics` returns no diagnostics +- Test isolated: `bun test packages/chess/src/modifiers/primitives/on-check-received.test.ts` → 5 pass, 0 fail (71ms) + +### Files touched +- NEW: `packages/chess/src/modifiers/primitives/on-check-received.ts` (74 lines) +- NEW: `packages/chess/src/modifiers/primitives/on-check-received.test.ts` (105 lines, 5 tests) +- `types.ts`: +1 line in `PrimitiveKind` union ("on-check-received" between "on-promotion" and "conditional") +- `index.ts`: +1 side-effect import (`./on-check-received.js`, after on-promotion) + +### Parallel-edit collision observed +- types.ts and index.ts were modified between my initial read and my first Edit attempt (on-promotion + on-turn-end agents landed concurrently). `mcp_Edit` correctly rejected with "File has been modified" — mitigation: re-read, re-apply edit against current state. Zero lost work. + +### Out of scope (T12 will handle) +- Dispatcher wiring: compare `getPreMoveCheckState(engine)` snapshot vs post-move re-captured check state for each royal; fire hooks only on false→true transition; filter to royals via `engine.getActiveRoyalEntityIds(color)` (with king-fallback per T4's captureCheckStateForColor pattern) +- Behaviour beyond seeding the attr + +--- + +## T-on-move Execution (2026-04-21) — seeding-only primitive + +### Shape (mirrors on-capture.ts, 72 lines) +- kind: `"on-move"`, label: `"On Move"`, seedsAttrs: `["OnMoveHooks"]` +- paramsSchema: `z.object({ primitives: z.array(NodeSchema) })` +- apply(): reads `ctx.session.get(pieceId, "OnMoveHooks")` → appends `[...params.primitives]` +- childPrimitives(): returns `[...params.primitives]` + +### longDescription emphasises Position-change semantic +- "fires whenever this piece's Position WME changes — normal moves, captures, castling-rook relocations, and en-passant pawn advances all count" +- "Fires AFTER the move resolves, on the mover itself" + +### 2 examples per spec +1. "Berserker — stacking attack on every move" (add-to-attribute AttackBonus +1) +2. "Nomad — heals 1 HP per step" (add-to-attribute Hp +1) + +### Test coverage (7 tests, 10 expects — seeding-only) +Scenarios named per user-facing spec but all expressed as seeding assertions (T21 will handle end-to-end firing): +- registry: kind present, instance identity, seedsAttrs declaration +- "fires when piece moves" — one apply → one hook in array +- "fires on captures too" — two sequential applies → hook list accumulates +- "fires on castling rook" — apply on separate pieceId seeds only that entity; king entity untouched +- "does NOT fire when piece is static" — opponent entity has no attr +- childPrimitives() returns inner list + +### ParamField snapshot coverage +`packages/chess/src/ui/ParamField.snapshot.test.tsx` has `SAMPLE_PARAMS: Record`. Added the `"on-move"` entry (required by union extension — my change forces it). On-turn-end / on-promotion / on-check-{received,delivered} keys are still missing and belong to sibling in-flight tasks. + +### Parallel-task interference in `bun run check` +- `bun run check` shows typecheck errors only from sibling in-flight tasks: on-check-delivered, on-check-received, on-promotion, on-turn-end (each pending either union entry or ParamField key). +- Zero `on-move` errors after my changes (verified with `tsc -b` clean-cache rerun). +- `lsp_diagnostics` clean on both new files. +- Isolated test: `bun test packages/chess/src/modifiers/primitives/on-move.test.ts` → 7 pass, 0 fail. +- Full primitives suite: 147 pass, 0 fail. +- docs.test.ts: 39 pass (16 primitives now registered, still ≥15). + +### Files touched +- NEW: `packages/chess/src/modifiers/primitives/on-move.ts` (72 lines) +- NEW: `packages/chess/src/modifiers/primitives/on-move.test.ts` (117 lines, 7 tests) +- `types.ts`: +1 line in `PrimitiveKind` union (`"on-move"` between `"on-capture"` and `"on-damaged"`) +- `index.ts`: +1 side-effect import (`./on-move.js`, after on-capture) +- `ParamField.snapshot.test.tsx`: +1 entry in `SAMPLE_PARAMS` (on-move sample between on-capture and on-damaged) + +### Out of scope (T12 will handle) +- Evaluator that fires OnMoveHooks on every Position WME change (quiet / capture / castling-rook / en-passant) +- End-to-end firing test (lands in T21) + +--- + +## on-turn-end primitive — 2026-04-21 + +### Files created +- `packages/chess/src/modifiers/primitives/on-turn-end.ts` (75 lines) — mirrors on-turn-start.ts +- `packages/chess/src/modifiers/primitives/on-turn-end.test.ts` (95 lines, 6 tests) + +### Schema divergence from on-turn-start +- on-turn-start params: `{ primitives: [...] }` (no color filter) +- on-turn-end params: `{ color: "white"|"black"|"both", primitives: [...] }` (color added per plan) +- Stored attr shape `OnTurnEndHooks: EffectPrimitiveNode[][]` — DOES NOT carry color +- Rationale: T21 dispatcher handles color filtering at fire-time; apply() drops color and seeds bare primitives (matches on-turn-start's storage shape exactly) +- **Tradeoff / flag for T21**: color info not round-trippable from stored attr. If T21 needs per-hook color, the attr shape must change to `[{color, primitives}]` (schema.ts edit). Current apply() drops it silently — `both` is the only safe semantic at fire-time until schema changes. + +### Wire-up required +1. `types.ts` PrimitiveKind union: added `| "on-turn-end"` +2. `index.ts` trigger group imports: added `import "./on-turn-end.js";` +3. `ParamField.snapshot.test.tsx` SAMPLE_PARAMS record: added `"on-turn-end": { color: "both", primitives: [...] }` entry (required because `Record` exhaustiveness check). New snapshot auto-generated on first test run. + +### Tests (6 total) +- registry registration (1) +- apply() seeds attr (1) +- apply() stacks across calls (1) +- paramsSchema rejects unknown color (1) +- paramsSchema accepts all 3 valid colors (1) — looped +- childPrimitives returns inner list (1) + +### Gotcha: stale `tsc -b` incremental cache +- `tsc -b` persists `packages/chess/tsconfig.tsbuildinfo` and reported stale errors about "on-turn-end" being missing from PrimitiveKind AFTER I added it to the union. +- **Fix**: `rm packages/chess/tsconfig.tsbuildinfo` then rerun typecheck. +- LSP-level diagnostics (`mcp_Lsp_diagnostics`) use a separate in-process tsserver and were clean immediately — the LSP/CLI mismatch is a reliable signal that the issue is the build cache, not actual type errors. +- Recommend wave-2 runners drop the buildinfo whenever extending `PrimitiveKind`. + +### Verification +- `bun test packages/chess/src/modifiers/primitives/on-turn-end.test.ts` → 6 pass, 0 fail (~86ms) +- `bun test packages/chess/src/ui/ParamField.snapshot.test.tsx` → 15/15 pass (1 new snapshot added) +- LSP: clean on on-turn-end.ts, on-turn-end.test.ts, types.ts, index.ts, ParamField.snapshot.test.tsx +- `bun run check` still fails on pre-existing parallel-task errors (on-promotion/on-check-received/on-check-delivered union entries missing + ParamField SAMPLE_PARAMS entries missing) — all my on-turn-end errors cleared. + +### Out of scope (T12 dispatcher + T21 E2E) +- Evaluator that fires OnTurnEndHooks at end of mover's turn, before opponent's on-turn-start +- Color filtering (white/black/both) at fire-time — currently dropped in apply() +- End-to-end firing ordering test (lands in T21) + +--- + +## T9 (on-check-delivered) — 2026-04-21 + +### Files added +- `packages/chess/src/modifiers/primitives/on-check-delivered.ts` (~80 lines) — verbatim mirror of on-capture.ts; kind `"on-check-delivered"`, seedsAttrs `["OnCheckDeliveredHooks"]`, 2 examples (Vampire-on-check / Stun-the-king). +- `packages/chess/src/modifiers/primitives/on-check-delivered.test.ts` (~110 lines, 8 tests): registry(2) + apply seed(1) + apply stacks(1) + Zod validates(3) + childPrimitives(1). +- `types.ts`: +1 line in `PrimitiveKind` union (`"on-check-delivered"`, placed after `"on-check-received"`). +- `index.ts`: +1 side-effect import (`./on-check-delivered.js`, placed after `./on-check-received.js`). +- `ParamField.snapshot.test.tsx`: +1 SAMPLE_PARAMS entry to keep `Record` exhaustiveness satisfied (no `it(...)` block added; on-move also skipped adding one — follow T6's choice for consistency). + +### Semantics pinned in longDescription +- Edge-triggered on the piece whose threat-line NEWLY reaches enemy royal +- Discovered check attribution: fires on the REVEALING attacker (line-of-sight unblocked), NOT the mover — critical divergence from naive "the mover delivered check" +- Double-check: fires on BOTH newly-attacking pieces +- Explicit reference to T12 evaluator using PRE_MOVE_CHECK_STATE_SNAPSHOTS diff (T4's attacker-ID-tracking snapshot is what makes discovered-check attribution tractable) + +### Verification +- `bun test packages/chess/src/modifiers/primitives/on-check-delivered.test.ts` → 8 pass (73ms) +- LSP diagnostics clean on both new files +- My added `on-check-delivered` kind no longer appears in typecheck errors after adding ParamField SAMPLE_PARAMS entry + +### Pre-existing test drift (NOT my scope) +- `bun run check` fails with pre-existing errors from concurrent sibling tasks that have not yet updated their touchpoints: + - `on-promotion` / `on-check-received` / `on-moved-onto-square` missing from `ParamField.snapshot.test.tsx`'s SAMPLE_PARAMS + - `on-captured.test.ts` imports `./on-captured.js` which doesn't exist yet (and uses `"on-captured"` kind not yet in union) + - `on-moved-onto-square.ts` has a SquareFilter type mismatch (discriminated union drift T2 vs T10) + - `BoardDiagramView.tsx` has 5 errors from T17 partial drop-in +- Baseline (git stash) showed 10 errors before my changes — I REDUCED the count by adding my kind properly, but other siblings have not yet landed their wire-ups. This is the expected "atomic per-primitive" churn pattern of this multi-agent sprint. + +### Out of scope (T12 + T21) +- Evaluator that fires OnCheckDeliveredHooks using pre/post attacker-set diff on royal squares +- Discovered-check / double-check firing-ordering test (lands in T21) +## Legacy descriptor backward-compat fixture — 2026-04-21 + +### Files added +- `packages/chess/src/modifiers/custom/__fixtures__/legacy-descriptor.json` — 15 top-level primitives covering ALL 15 pre-Wave-2 legacy kinds exactly once; depth 2 (on-capture/on-damaged/on-turn-start wrap 1 nested primitive, conditional wraps `then[0]` + `else[0]`). Total primitive node count including children = 20 (well under cap of 50). +- `packages/chess/src/modifiers/custom/legacy-descriptor.test.ts` — 4 scenarios (parse / validate / apply / serialize round-trip). 37 expect() calls. + +### Fixture loading pattern chosen +- Used `fs.readFileSync` via `import.meta.url` → `fileURLToPath` → `dirname` → `join(..., '__fixtures__', 'legacy-descriptor.json')`. Rejected `import fixture from "./__fixtures__/legacy-descriptor.json" with { type: "json" }` because tsconfig does NOT enable `resolveJsonModule` and repo has zero existing JSON-import usages in source tree. +- Side benefit: testing the exact bytes on disk is the cleanest proof for the round-trip assertion (the only cast is `JSON.parse(text) as unknown`). + +### CRITICAL: applyCustomDescriptor walks childPrimitives() recursively at profile-time +- `apply.ts#walkAndApply` descends into every primitive's `childPrimitives()` up to `RUNTIME_DEPTH_HARD_CAP=8`. +- This means nested primitives inside on-capture / on-damaged / on-turn-start / conditional ALSO run at apply-time, in addition to seeding their respective Hook attrs. +- First-write of test: assumed Hp=5*2=10 after seed+multiply. Actual: Hp=11 because on-turn-start's nested `add-to-attribute Hp +1` ran at apply-time too. +- Same pattern for HpBonus (outer +2, on-capture nested +1, conditional.else nested +1 = 4) and ReflectDamagePercent (outer 25 is overwritten by nested 50 inside on-damaged; last-write-wins per reflect-damage semantics). +- Tests now document this contract explicitly — valuable to future readers since it contradicts the naive "nested primitives only fire at trigger-time" intuition. + +### Results +- `bun test …legacy-descriptor.test.ts` → 4 pass / 0 fail / 37 expects (86ms). +- `bun run test` full suite: 1912 pass / 0 fail (was 1908 baseline; +4 from this work, aligned with task spec). +- `bun run typecheck` exit 0. +- `bun run lint` fails with 14 pre-existing errors in `packages/chess/src/modifiers/triggers.test.ts` (sibling-task unused-imports from T12 dispatcher wiring — baseline confirmed via `git stash` + `bun run lint`: identical output). My new files lint clean (`bunx eslint legacy-descriptor.test.ts` → 0 errors). +- `bun run check` fails solely because of the above lint failure. NOT caused by this work. + +### Primitives count in fixture +- 15 top-level + 5 nested = 20 total primitive nodes +- Depth 2 (max depth of any subtree) +- 15 distinct kinds (all legacy, zero Wave-2 additions) + +--- + +## BlockCard (T16) - 2026-04-21 +- Created `BlockCard.tsx` and `BlockCard.test.tsx` as pure presentational components without state management or dnd-kit imports (those will come in T18 via wrapping). +- Reused CATEGORIES map from `CustomModifierEditor.tsx` directly to style cards by kind (State=blue, Mechanic=emerald, Trigger=violet). +- Wrapped the `ParamField` inside a collapsible inspector view conditionally rendered on `isSelected` AND `isExpanded`. +- Rendered `childBlocks` prop directly when `isExpanded` is true and passed, allowing the orchestrator/wrapper to handle the recursive construction of nested cards. +- Visually clamped depth to 3 by scaling indentation (`visualDepth * 1.5rem`). +- A11y: Handled Enter to toggle expansion, Space to select, and Delete/Backspace to remove primitive. +- Snapshot testing with `renderToStaticMarkup` provides quick, deterministic validations for UI presence without requiring heavy simulated rendering with `@testing-library/react` events in pure presentational layers. + +--- + +## T12 Execution (2026-04-21) — fire*Hooks evaluators + +### runPrimitives signature change +- Old: `runPrimitives(engine, pieceId, nodes, depth)` +- New: `runPrimitives(engine, pieceId, nodes, depth, event?: PrimitiveEvent)` +- Backward compatible: existing 4 callers pass no event → context gets `event: undefined`. +- Recursive child walk threads the SAME event into nested levels so primitives at any depth see the trigger metadata that fired the root. + +### on-turn-end color storage — picked option (a) (schema change) +- Extended `OnTurnEndHooks` from `readonly EffectPrimitiveNode[][]` to `readonly { color: "white"|"black"|"both"; primitives: readonly EffectPrimitiveNode[] }[]`. +- Updated `schema.ts`, `on-turn-end.ts` apply(), `on-turn-end.test.ts` (changed expected attr shape in 2 assertions). +- Rationale: option (b) "fire for all colors and call color advisory" silently breaks the user-facing `color: "white"` semantic from the primitive's params. Option (a) preserves the contract end-to-end. + +### Circular-import handling — passed snapshots as parameters +- `apply.ts` already imports from `triggers.ts` (fireOn* functions). Importing `getPreMoveCheckState` BACK from apply.ts would form a cycle. +- Solution: `fireOnCheckReceivedHooks(engine, preMoveCheckState)` and `fireOnCheckDeliveredHooks(engine, preMoveCheckState)` accept the snapshot as a parameter. T21's onAfterMove will read it via `getPreMoveCheckState(engine)` and pass it in — mirrors the `fireOnDamagedHooks(engine, preHp)` pattern. +- Bonus: structural type `PreMoveCheckStateLike` re-declared in triggers.ts (interface-compatible with apply.ts's `PreMoveCheckState`) — avoids the cycle while keeping the contract typed. +- Re-implemented `computeCheckStateForColor` locally in triggers.ts (verbatim copy of `apply.ts#captureCheckStateForColor`) for the post-move probe. The two implementations are intentional duplicates — keep them in sync if either changes (TODO: post-T21, consider extracting to `pre-move-state.ts` shared module). + +### fireOnCapturedHooks targeting +- Each entry in `OnCapturedHooks` carries `{target: TargetResolver, primitives: ...}`. +- Build a transient `PrimitiveApplyContext` pinned to the dying piece (`pieceId: capturedPieceId`) JUST FOR `resolveTargets()` — its `event` carries `{kind:'capture', attackerId, defenderId}` so `target: 'attacker'`/`'defender'` resolve correctly AND `relation: 'ally'/'enemy'` resolve relative to the defender. +- Then run primitives with each resolved target as `pieceId` (a separate `PrimitiveApplyContext` is built per target inside `runPrimitives`). +- Event is THREADED into runPrimitives so nested primitives at any depth still see capture metadata. + +### Tests added (13 new) +- fireOnMoveHooks: 2 (fires-for-moved + skips-non-moved) +- fireOnTurnEndHooks: 1 (color filter — covers white/black/both in one test) +- fireOnPromotionHooks: 2 (fires + no-attr no-op) +- fireOnCheckReceivedHooks: 1 (edge transition + non-edge no-op in same test) +- fireOnCheckDeliveredHooks: 2 (discovered-check attribution + already-attacking no-op) +- fireOnMovedOntoSquareHooks: 3 (squares list, rank-only predicate, file+rank predicate) +- fireOnCapturedHooks: 2 (target='attacker' redirection + target='self' default) + +Tests call evaluators DIRECTLY because T21 hasn't wired them into onAfterMove yet — that's intentional, the task spec is explicit about it. + +### Verification +- `bun test packages/chess/src/modifiers/triggers.test.ts` → 20 pass (was 7), 41 expect() calls +- `bun test packages/chess` → 1909 pass / 91 fail / 88 errors (vs baseline 1896 pass / 91 fail / 88 errors → +13 pass, ZERO new failures). Pre-existing failures are Playwright/asset/preset-registry/prediction-manager noise unrelated to triggers. +- `bun run check` → exit 0, 1925 vitest tests pass. + +### Files touched +- MODIFIED: `packages/chess/src/schema.ts` (OnTurnEndHooks shape extended) +- MODIFIED: `packages/chess/src/modifiers/primitives/on-turn-end.ts` (apply stores color) +- MODIFIED: `packages/chess/src/modifiers/primitives/on-turn-end.test.ts` (2 assertions match new shape) +- REWROTE: `packages/chess/src/modifiers/triggers.ts` (228 → 540 lines: +7 evaluators, runPrimitives event threading, computeCheckStateForColor mirror) +- MODIFIED: `packages/chess/src/modifiers/triggers.test.ts` (+13 tests, +330 lines) + +### Out of scope (T21) +- Wiring the 7 new evaluators into apply.ts onAfterMove +- Computing `movedPieceIds` from Position-diff snapshot +- Computing promoted-piece + (from, to) from PieceType-diff snapshot +- Sequencing fireOnCapturedHooks BEFORE retraction + +Added BlockList wrapping BlockCard with dnd-kit for sorting. Added tests verifying DOM structure and events. Added accessibility announcements. Handled nested SortableContext instances via recursion (with explicit depth passing). Avoided modifying BlockCard logic directly, kept BlockList pure. + +--- + +## T21 Execution (2026-04-21) — Metis-locked dispatch wiring + +### onAfterMove now runs the 12-stage sequence +1 computeAuraFacts → 2 fireOnDamagedHooks → 3 fireOnCaptureHooks → 4 fireOnCapturedHooks → 5 fireOnPromotionHooks → 6 fireOnMoveHooks → 7 fireOnMovedOntoSquareHooks (per moved piece) → 8 fireOnCheckReceivedHooks → 9 fireOnCheckDeliveredHooks → 10 fireConditionalHooks → 11 fireOnTurnEndHooks(ctx.mover) → 12 fireOnTurnStartHooks(opposite-of-mover). + +### New WeakMaps + diff helpers in apply.ts +- `PRE_MOVE_POSITION_SNAPSHOTS: WeakMap>` — populated in onBeforeMove via `snapshotPositions(session)`. Diffed against post-move via `diffMovedPieceIds()` (excludes retracted pieces — they trigger on-captured instead, per the design notes in triggers.ts). +- `PRE_MOVE_CAPTURED_DEFENDERS: WeakMap` — populated alongside `PRE_MOVE_CAPTURE_ATTACKERS` using `getPieceAt(session, ctx.to as Square)`. NB the BeforeMoveContext field is `ctx.to` (number), NOT `ctx.toSquare` — the task spec said `ctx.toSquare` but the actual interface (registry.ts:159-165) exposes `from`/`to`/`isCapture`/`pieceId`/`mover`. +- Added 2 helper diff funcs at module scope: `diffMovedPieceIds` and `diffPromotedPieces`. Both pure, both filter retracted facts via `session.get(...) === undefined` early-skip. + +### En-passant defender NOT covered (documented limitation) +- EP captures the pawn on a different square than `ctx.to`. The current PRE_MOVE_CAPTURED_DEFENDERS lookup uses `getPieceAt(session, ctx.to)` only, which finds the destination square (empty for EP). For now, EP victims do NOT fire on-captured. Documented in the WeakMap docstring; future fix would require `BeforeMoveContext` to expose `epVictimSquare` or for the dispatcher to peek at `engine.moveLog` post-move. + +### "BEFORE retraction" naming is aspirational, NOT literal +- The task description says "fireOnCapturedHooks BEFORE retraction" but the engine's actual fact-retraction for the captured piece happens INSIDE `applyMove` BEFORE the onAfterMove hook fires. So by the time we call fireOnCapturedHooks the defender's facts are already gone. The contract the dispatcher CALL ORDER actually enforces is: "fire the dying piece's hook list before any sibling dispatcher could re-seed/mutate it" — not "before its facts disappear." Inner primitives that need to read defender attrs MUST consult `event.defenderId` in the trigger ctx, not session reads. Documented in apply.ts onAfterMove docstring. + +### finally block clears all 4 pre-move snapshots +- HP + capture-attackers cleared INSIDE the try (existing pattern preserved); check-state, promotion-pawns, position, captured-defenders ALL cleared in `finally` so any throw mid-pipeline can't leak state to the next move. + +### Test added — "T21 onAfterMove dispatch order (Metis-locked)" (3 tests) +- **Approach picked: `vi.spyOn(triggers, name).mockImplementation(wrapper)`** — wraps each of the 11 fire*Hooks dispatchers with a lambda that pushes the function name to a callLog AND calls through to the original. Captures inter-dispatcher call ORDER without breaking downstream behaviour. Cleanest of the three approaches the task suggested; doesn't require crafting a single move that fires all 11 trigger types. +- 3 sub-tests: quiet move (e2-e4 → 9 names, on-captured + on-promotion guarded out), capture (d4xe5 → 10 names, on-promotion guarded), promotion (a7-a8=Q → 10 names, on-captured guarded). +- `firstOccurrences()` helper collapses repeated dispatcher names (e.g. fireOnMovedOntoSquareHooks runs once per moved piece) so the assertion compares INTER-dispatcher ordering only. +- Important: cache `original = triggers[name]` BEFORE installing the spy — `vi.spyOn` REPLACES the export with the spy, so reading `triggers[name]` inside the wrapper would recurse infinitely. + +### Verification +- `bun test packages/chess/src/modifiers/apply.test.ts` → 14/14 pass (was 11; +3 from T21) +- `bun test packages/chess/src/modifiers/triggers.test.ts` → 20/20 pass (T12 unchanged) +- `bun test packages/chess/src/modifiers/custom/legacy-descriptor.test.ts` → 4/4 pass (T20 unchanged) +- `bun run check` → exit 0; **1932 tests pass** (1925 baseline + 7 from this work and other in-flight tasks landing concurrently) + +### Files touched +- `packages/chess/src/modifiers/apply.ts` — +7 fire* imports; +2 WeakMaps; +3 helper functions (snapshotPositions, diffMovedPieceIds, diffPromotedPieces); onBeforeMove +2 snapshot writes; onAfterMove rewritten to 12-stage sequence; finally block +2 deletes +- `packages/chess/src/modifiers/apply.test.ts` — +import `* as triggers`; +`afterEach`; +describe "T21 onAfterMove dispatch order" with 3 tests (~155 lines) +- Use generateDefaultParams logic with proper object shape initialization including nested schema resolution (enum options, arrays, nested defaults) to prevent runtime crashes when initializing new primitives. +- ZodEnum requires type assertion casting `(subSchema as unknown as { options: string[] }).options[0]` in TS when interacting generically. + +## T20 - Form/Visual mode toggle +- Replaced right panels of CustomModifierEditor with VisualBuilderPane in visual mode +- Used exact localStorage key `houserules:custom-modifier-editor-mode:v1` +- Mocked localStorage directly in `CustomModifierEditor.test.tsx` because `@vitest-environment happy-dom` combined with direct node execution didn't mock localStorage correctly +- Added mode toggles correctly using `aria-pressed` for a11y testing +- Left navigation tests alone since we did not modify e2e logic diff --git a/packages/chess/src/ui/CustomModifierEditor.test.tsx b/packages/chess/src/ui/CustomModifierEditor.test.tsx new file mode 100644 index 0000000..51ea5e2 --- /dev/null +++ b/packages/chess/src/ui/CustomModifierEditor.test.tsx @@ -0,0 +1,71 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { renderToStaticMarkup } from 'react-dom/server'; +import { CustomModifierEditor } from './CustomModifierEditor.js'; +import React from 'react'; + +// Mock localStorage since we're in a Node environment for SSR tests +const localStorageMock = (() => { + let store: Record = {}; + return { + getItem: vi.fn((key: string) => store[key] ?? null), + setItem: vi.fn((key: string, value: string) => { + store[key] = value.toString(); + }), + clear: vi.fn(() => { + store = {}; + }), + removeItem: vi.fn((key: string) => { + delete store[key]; + }) + }; +})(); + +Object.defineProperty(global, 'localStorage', { + value: localStorageMock +}); + +describe('CustomModifierEditor', () => { + const LOCAL_STORAGE_KEY = 'houserules:custom-modifier-editor-mode:v1'; + + beforeEach(() => { + localStorage.clear(); + vi.clearAllMocks(); + }); + + afterEach(() => { + localStorage.clear(); + }); + + function renderEditor() { + return renderToStaticMarkup( + {}} /> + ); + } + + it("default mode is 'form' when localStorage empty", () => { + const html = renderEditor(); + + // Toggle buttons should reflect form is active + expect(html).toContain('data-testid="custom-modifier-editor-mode-form" aria-pressed="true"'); + expect(html).toContain('data-testid="custom-modifier-editor-mode-visual" aria-pressed="false"'); + + // Should render the tree view / inspector (form mode components) + expect(html).toContain('Effect Sequence'); + expect(html).toContain('Parameter Inspector'); + }); + + it("reading mode='visual' from localStorage renders VisualBuilderPane", () => { + localStorage.setItem(LOCAL_STORAGE_KEY, 'visual'); + const html = renderEditor(); + + // Toggle buttons should reflect visual is active + expect(html).toContain('data-testid="custom-modifier-editor-mode-form" aria-pressed="false"'); + expect(html).toContain('data-testid="custom-modifier-editor-mode-visual" aria-pressed="true"'); + + // Should render VisualBuilderPane content, not form components + expect(html).not.toContain('Effect Sequence'); + expect(html).not.toContain('Parameter Inspector'); + // VisualBuilderPane has 'Primitives' text and BlockList container + expect(html).toContain('Primitives'); + }); +}); diff --git a/packages/chess/src/ui/CustomModifierEditor.tsx b/packages/chess/src/ui/CustomModifierEditor.tsx index 4b37bb6..12d995c 100644 --- a/packages/chess/src/ui/CustomModifierEditor.tsx +++ b/packages/chess/src/ui/CustomModifierEditor.tsx @@ -15,6 +15,7 @@ import { CUSTOM_MODIFIER_RECIPES, type CustomModifierRecipe, } from '../modifiers/custom/recipes.js'; +import { VisualBuilderPane } from './visual-builder/VisualBuilderPane.js'; import { ParamField } from './ParamField.js'; interface Props { @@ -126,6 +127,24 @@ export function CustomModifierEditor({ isOpen, onClose, onShareWithRoom }: Props // Templates picker state (built-in recipe gallery) const [showTemplates, setShowTemplates] = useState(false); + const [mode, setMode] = useState<'form' | 'visual'>(() => { + try { + const stored = localStorage.getItem('houserules:custom-modifier-editor-mode:v1'); + return stored === 'visual' ? 'visual' : 'form'; + } catch { + return 'form'; + } + }); + + const setModeAndPersist = (m: 'form' | 'visual') => { + setMode(m); + try { + localStorage.setItem('houserules:custom-modifier-editor-mode:v1', m); + } catch { + // ignore + } + }; + // Open library const openLibrary = () => { setLibraryItems(loadCustomModifierLibrary()); @@ -220,6 +239,32 @@ export function CustomModifierEditor({ isOpen, onClose, onShareWithRoom }: Props
+
+ + +
); - } - const tooltip = tooltipParts.join(''); - return ( - - ); - })} + })} +
- - ))} - + ))} + + )} - {/* Center: Tree View */} -
-

- Effect Sequence ({descriptor.primitives.length}) -

- {descriptor.primitives.length === 0 ? ( -
- Add primitives from the palette to build the modifier. -
- ) : ( -
- {descriptor.primitives.map((node, i) => { - const isSelected = selectedIndex === i; - const primitive = PRIMITIVE_REGISTRY.get(node.kind); - return ( -
setSelectedIndex(i)} - className={` - flex items-center justify-between p-3 rounded-lg border cursor-pointer transition-colors - ${isSelected - ? 'border-blue-500 bg-blue-50 shadow-sm ring-1 ring-blue-500/20' - : 'border-neutral-200 hover:border-neutral-300 bg-white hover:bg-neutral-50'} - `} - > -
-
- {i + 1} -
-
-
- {primitive?.label ?? node.kind} -
-
- {JSON.stringify(node.params).substring(0, 60)} - {JSON.stringify(node.params).length > 60 ? '...' : ''} -
-
-
- -
- ); - })} -
- )} -
- - {/* Right: Inspector */} - + + ) : ( + <> + {/* Center: Tree View */} +
+

+ Effect Sequence ({descriptor.primitives.length}) +

+ {descriptor.primitives.length === 0 ? ( +
+ Add primitives from the palette to build the modifier. +
+ ) : ( +
+ {descriptor.primitives.map((node, i) => { + const isSelected = selectedIndex === i; + const primitive = PRIMITIVE_REGISTRY.get(node.kind); + return ( +
setSelectedIndex(i)} + className={` + flex items-center justify-between p-3 rounded-lg border cursor-pointer transition-colors + ${isSelected + ? 'border-blue-500 bg-blue-50 shadow-sm ring-1 ring-blue-500/20' + : 'border-neutral-200 hover:border-neutral-300 bg-white hover:bg-neutral-50'} + `} + > +
+
+ {i + 1} +
+
+
+ {primitive?.label ?? node.kind} +
+
+ {JSON.stringify(node.params).substring(0, 60)} + {JSON.stringify(node.params).length > 60 ? '...' : ''} +
+
+
+ +
+ ); + })} +
+ )} +
+ + {/* Right: Inspector */} + + + )} {/* Footer: Validation Status */}