diff --git a/docs/superpowers/plans/2026-03-27-computed-local-target.md b/docs/superpowers/plans/2026-03-27-computed-local-target.md new file mode 100644 index 0000000..84472d4 --- /dev/null +++ b/docs/superpowers/plans/2026-03-27-computed-local-target.md @@ -0,0 +1,319 @@ +# Computed `@`-Target Support Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Allow `{computed @var = expr}` to write per-iteration values into the local scope, fixing issue #140 (computed values shared across `{for}` iterations). + +**Architecture:** Extend `parseComputedArgs` to accept `@` prefix, add a `localsUpdate` write path to `computeAndApply`, and wire it to `ctx.update` (which is already bound to the current `LocalsUpdateContext` by `defineMacro`). + +**Tech Stack:** Preact, TypeScript, Vitest (happy-dom) + +--- + +### Task 1: Failing tests for `@`-target computed inside `{for}` + +**Files:** + +- Modify: `test/dom/macros.test.tsx:297-320` (add new tests after existing `{computed}` tests) + +- [ ] **Step 1: Add test — single iteration with `@` target** + +Add this test inside the `describe('{for}')` block, after the existing computed tests (after line 320): + +```tsx +it('computed with @-target writes to local scope per-iteration', () => { + useStoryStore.getState().setTemporary('items', [{ name: 'a', status: 'ok' }]); + + const container = document.createElement('div'); + const passage = makePassage( + 1, + 'Test', + "{for @item of _items}{computed @cls = @item.status == 'ok' ? 'green' : 'red'}{@item.name}-{@cls}{/for}", + ); + act(() => { + render(, container); + }); + + const results = container.querySelectorAll('.result'); + expect(results).toHaveLength(1); + expect(results[0].textContent).toBe('a-green'); +}); +``` + +- [ ] **Step 2: Add test — multiple iterations with `@` target (the actual bug)** + +Add immediately after the previous test: + +```tsx +it('computed @-target produces per-iteration values in multi-item for-loop (#140)', () => { + useStoryStore.getState().setTemporary('items', [ + { name: 'a', status: 'ok' }, + { name: 'b', status: 'err' }, + ]); + + const container = document.createElement('div'); + const passage = makePassage( + 1, + 'Test', + "{for @item of _items}{computed @cls = @item.status == 'ok' ? 'green' : 'red'}{@item.name}-{@cls}{/for}", + ); + act(() => { + render(, container); + }); + + const results = container.querySelectorAll('.result'); + expect(results).toHaveLength(2); + expect(results[0].textContent).toBe('a-green'); + expect(results[1].textContent).toBe('b-red'); +}); +``` + +- [ ] **Step 3: Add test — `@` target outside local scope logs error** + +Add immediately after the previous test: + +```tsx +it('computed @-target outside local scope logs error', () => { + const spy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const container = document.createElement('div'); + const passage = makePassage(1, 'Test', "{computed @cls = 'foo'}"); + act(() => { + render(, container); + }); + + expect(spy).toHaveBeenCalledWith( + expect.stringContaining('{computed @cls'), + expect.any(Error), + ); + spy.mockRestore(); +}); +``` + +Note: this test also needs `vi` imported. Check line 2 — if `vi` is not in the import, add it: + +```tsx +import { describe, it, expect, beforeEach, vi } from 'vitest'; +``` + +- [ ] **Step 4: Run tests to verify they fail** + +Run: `npx vitest run test/dom/macros.test.tsx --reporter=verbose 2>&1 | tail -30` + +Expected: All three new tests FAIL. The first two fail with `{computed}: target must be $name or _name, got "@cls"`. The third may also fail since the error message differs. + +- [ ] **Step 5: Commit** + +```bash +git add test/dom/macros.test.tsx +git commit -m "test: add failing tests for computed @-target in for-loops (#140)" +``` + +--- + +### Task 2: Implement `@`-target support in `{computed}` + +**Files:** + +- Modify: `src/components/macros/Computed.tsx` + +- [ ] **Step 1: Expand parser to accept `@` prefix** + +In `Computed.tsx`, change line 25 from: + +```tsx +if (!target.match(/^[$_]\w+$/)) { + throw new Error(`{computed}: target must be $name or _name, got "${target}"`); +} +``` + +to: + +```tsx +if (!target.match(/^[$_@]\w+$/)) { + throw new Error( + `{computed}: target must be $name, _name, or @name, got "${target}"`, + ); +} +``` + +- [ ] **Step 2: Add `localsUpdate` parameter to `computeAndApply`** + +Replace the `computeAndApply` function (lines 57-85) with: + +```tsx +function computeAndApply( + expr: string, + name: string, + isTemp: boolean, + isLocal: boolean, + variables: Record, + temporary: Record, + locals: Record, + transient: Record, + rawArgs: string, + prevRef: { current: unknown }, + localsUpdate: ((key: string, value: unknown) => void) | null, +): void { + let newValue: unknown; + try { + newValue = evaluate(expr, variables, temporary, locals, transient); + } catch (err) { + console.error( + `spindle: Error in {computed ${rawArgs}}${currentSourceLocation()}:`, + err, + ); + return; + } + + if (!valuesEqual(prevRef.current, newValue)) { + prevRef.current = newValue; + if (isLocal) { + try { + localsUpdate!(name, newValue); + } catch (err) { + console.error( + `spindle: Error in {computed ${rawArgs}}${currentSourceLocation()}:`, + err, + ); + } + } else { + const state = useStoryStore.getState(); + if (isTemp) state.setTemporary(name, newValue); + else state.setVariable(name, newValue); + } + } +} +``` + +- [ ] **Step 3: Wire `ctx.update` in the render function** + +Replace the render function body (lines 87-142) with: + +```tsx +defineMacro({ + name: 'computed', + merged: true, + render({ rawArgs }, ctx) { + const [mergedVars, mergedTemps, mergedLocals, mergedTrans] = ctx.merged!; + + let target: string; + let expr: string; + try { + ({ target, expr } = parseComputedArgs(rawArgs)); + } catch (err) { + return ( + + ); + } + const isLocal = target.startsWith('@'); + const isTemp = target.startsWith('_'); + const name = target.slice(1); + const localsUpdate = isLocal ? ctx.update : null; + + const prevOutput = ctx.hooks.useRef(undefined); + + const ran = ctx.hooks.useRef(false); + if (!ran.current) { + ran.current = true; + computeAndApply( + expr, + name, + isTemp, + isLocal, + mergedVars, + mergedTemps, + mergedLocals, + mergedTrans, + rawArgs, + prevOutput, + localsUpdate, + ); + } + + ctx.hooks.useLayoutEffect(() => { + computeAndApply( + expr, + name, + isTemp, + isLocal, + mergedVars, + mergedTemps, + mergedLocals, + mergedTrans, + rawArgs, + prevOutput, + localsUpdate, + ); + }, [mergedVars, mergedTemps, mergedLocals, mergedTrans]); + + return null; + }, +}); +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `npx vitest run test/dom/macros.test.tsx --reporter=verbose 2>&1 | tail -40` + +Expected: All tests pass, including the three new ones. + +- [ ] **Step 5: Run full test suite** + +Run: `npx vitest run --reporter=verbose 2>&1 | tail -20` + +Expected: All tests pass. No regressions. + +- [ ] **Step 6: Run typecheck** + +Run: `npx tsc --noEmit 2>&1 | tail -10` + +Expected: No type errors. + +- [ ] **Step 7: Commit** + +```bash +git add src/components/macros/Computed.tsx +git commit -m "fix: support @-target in {computed} for per-iteration values inside {for} (#140)" +``` + +--- + +### Task 3: Tighten existing test comment + +**Files:** + +- Modify: `test/dom/macros.test.tsx:313-315` + +- [ ] **Step 1: Update comment on existing `_var` multi-iteration test** + +Replace lines 313-315: + +```tsx +// Multiple iterations writing to the same _derived temp: last-write-wins, +// so both iterations see the final value. The key assertion is that it +// renders without hanging and produces the correct number of results. +``` + +with: + +```tsx +// _var targets write to global temp store — all iterations share the same +// variable, so last-write-wins. Use @-target for per-iteration values. +``` + +- [ ] **Step 2: Run tests to confirm nothing broke** + +Run: `npx vitest run test/dom/macros.test.tsx --reporter=verbose 2>&1 | tail -20` + +Expected: All tests pass. + +- [ ] **Step 3: Commit** + +```bash +git add test/dom/macros.test.tsx +git commit -m "test: clarify _var last-write-wins comment in for-loop computed test (#140)" +``` diff --git a/docs/superpowers/specs/2026-03-27-computed-local-target-design.md b/docs/superpowers/specs/2026-03-27-computed-local-target-design.md new file mode 100644 index 0000000..b577fb0 --- /dev/null +++ b/docs/superpowers/specs/2026-03-27-computed-local-target-design.md @@ -0,0 +1,77 @@ +# Design: Support `@` target variables in `{computed}` (issue #140 reopened) + +**Issue**: [#140](https://github.com/rohal12/spindle/issues/140) — reopened because v0.43.1 fixed the infinite loop but computed values are shared across `{for}` iterations +**Date**: 2026-03-27 +**Approach**: Support `@` target in `{computed}` via LocalsUpdateContext (Approach 1) + +## Problem + +`{computed _cls = @item.status == 'ok' ? 'green' : 'red'}` inside `{for @item of _items}` writes to the global temporary store. All iterations overwrite the same `_cls` variable — last-write-wins. The user expects per-iteration values. + +The v0.43.1 fix stabilized object references to prevent infinite loops, but didn't address the scoping issue. The `{computed}` macro only supports `$var` (story) and `_var` (temp) targets — both are global. There is no way to write a computed value into the per-iteration `@` local scope. + +## Solution + +Extend `{computed}` to accept `@var` targets. When the target uses the `@` prefix, write to the iteration's local scope via `LocalsUpdateContext` instead of the Zustand store. + +### Change 1: Parser — accept `@` prefix + +In `parseComputedArgs` (`Computed.tsx`), expand the target validation regex from `/^[$_]\w+$/` to `/^[$_@]\w+$/`. Update the error message to include `@name`. + +### Change 2: Write path — local scope via `ctx.update` + +Add parameters to `computeAndApply`: + +- `isLocal: boolean` — true when target starts with `@` +- `localsUpdate: ((key: string, value: unknown) => void) | null` — the `update` function from `LocalsUpdateContext` + +When `isLocal && localsUpdate`, call `localsUpdate(name, value)` instead of `state.setTemporary(name, value)` or `state.setVariable(name, value)`. + +The `prevRef` equality check (`valuesEqual`) still guards against infinite loops — if the value hasn't changed, no write occurs, no re-render cascade. + +### Change 3: Render function — detect `@` and pass `ctx.update` + +In the `{computed}` render function: + +- Detect `@` prefix on target +- Pass `ctx.update` to `computeAndApply` as `localsUpdate` when `isLocal` is true +- `ctx.update` is already wired to the current `LocalsUpdateContext` by `defineMacro` (line 147 of `define-macro.ts`) + +Each `ForIteration` has its own `LocalsUpdateContext.Provider`, so `ctx.update` naturally targets the correct iteration's scope. + +### Render cycle + +For `{computed @cls = @item.status == 'ok' ? 'green' : 'red'}` inside a 2-item `{for}`: + +1. Iteration 1 renders: `ran.current` block evaluates → `'green'` → `update('cls', 'green')` → schedules re-render +2. Iteration 2 renders: `ran.current` block evaluates → `'red'` → `update('cls', 'red')` → schedules re-render +3. Each iteration's `setLocalMutations` fires independently → each `localState` includes its own `cls` +4. `useLayoutEffect` fires in each → same values → `prevRef` prevents write → stable + +One extra render per `{computed @}` per iteration — same as existing `_var` behavior with Zustand store writes. + +### Error handling + +- **`{computed @cls = ...}` outside a local scope**: `ctx.update` comes from `defaultUpdater` in `render.tsx` which throws: "Cannot set @cls — local variables require a {for}, widget, {link}, or {button} scope". The write path in `computeAndApply` needs a try/catch around the `localsUpdate` call (the existing try/catch only wraps `evaluate()`). Caught errors are logged via `console.error`. +- **`{computed @cls = ...}` referencing undefined `@` vars in expression**: already handled by `evaluate()`. + +No new error paths needed. + +## Testing + +1. **Single iteration, `@` target**: `{computed @cls = @item.status == 'ok' ? 'green' : 'red'}` with one item → `@cls` is `'green'` +2. **Multiple iterations, `@` target** (the actual bug): two items → first iteration `'green'`, second `'red'` — per-iteration isolation +3. **Existing `_var` multi-iteration test**: keep current relaxed assertion to document last-write-wins for `_var` targets, tighten comment +4. **`@` target outside local scope**: `{computed @cls = 'foo'}` at passage level → renders error via console.error + +## Files changed + +- `src/components/macros/Computed.tsx` — all three changes above +- `test/dom/macros.test.tsx` — test cases 1, 2, 4; update comment on existing test + +## Out of scope + +- Changes to `For.tsx`, `WidgetInvocation.tsx`, or `useMergedLocals` +- Changes to `{set}` or other macros +- Auto-scoping `_var` targets inside loops +- Inline reactive expressions diff --git a/src/components/macros/Computed.tsx b/src/components/macros/Computed.tsx index 1b47f28..672db41 100644 --- a/src/components/macros/Computed.tsx +++ b/src/components/macros/Computed.tsx @@ -22,9 +22,9 @@ function parseComputedArgs(rawArgs: string): { target: string; expr: string } { const target = trimmed.slice(0, i).trim(); const expr = trimmed.slice(i + 1).trim(); - if (!target.match(/^[$_]\w+$/)) { + if (!target.match(/^[$_@]\w+$/)) { throw new Error( - `{computed}: target must be $name or _name, got "${target}"`, + `{computed}: target must be $name, _name, or @name, got "${target}"`, ); } @@ -58,12 +58,14 @@ function computeAndApply( expr: string, name: string, isTemp: boolean, + isLocal: boolean, variables: Record, temporary: Record, locals: Record, transient: Record, rawArgs: string, prevRef: { current: unknown }, + localsUpdate: ((key: string, value: unknown) => void) | null, ): void { let newValue: unknown; try { @@ -78,9 +80,20 @@ function computeAndApply( if (!valuesEqual(prevRef.current, newValue)) { prevRef.current = newValue; - const state = useStoryStore.getState(); - if (isTemp) state.setTemporary(name, newValue); - else state.setVariable(name, newValue); + if (isLocal) { + try { + localsUpdate!(name, newValue); + } catch (err) { + console.error( + `spindle: Error in {computed ${rawArgs}}${currentSourceLocation()}:`, + err, + ); + } + } else { + const state = useStoryStore.getState(); + if (isTemp) state.setTemporary(name, newValue); + else state.setVariable(name, newValue); + } } } @@ -102,8 +115,10 @@ defineMacro({ /> ); } + const isLocal = target.startsWith('@'); const isTemp = target.startsWith('_'); const name = target.slice(1); + const localsUpdate = isLocal ? ctx.update : null; const prevOutput = ctx.hooks.useRef(undefined); @@ -114,12 +129,14 @@ defineMacro({ expr, name, isTemp, + isLocal, mergedVars, mergedTemps, mergedLocals, mergedTrans, rawArgs, prevOutput, + localsUpdate, ); } @@ -128,12 +145,14 @@ defineMacro({ expr, name, isTemp, + isLocal, mergedVars, mergedTemps, mergedLocals, mergedTrans, rawArgs, prevOutput, + localsUpdate, ); }, [mergedVars, mergedTemps, mergedLocals, mergedTrans]); diff --git a/test/dom/macros.test.tsx b/test/dom/macros.test.tsx index c85c500..8e0275c 100644 --- a/test/dom/macros.test.tsx +++ b/test/dom/macros.test.tsx @@ -1,5 +1,5 @@ // @vitest-environment happy-dom -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; import { render } from 'preact'; import { act } from 'preact/test-utils'; import { Passage } from '../../src/components/Passage'; @@ -310,14 +310,71 @@ describe('macro components', () => { render(, container); }); - // Multiple iterations writing to the same _derived temp: last-write-wins, - // so both iterations see the final value. The key assertion is that it - // renders without hanging and produces the correct number of results. + // _var targets write to global temp store — all iterations share the same + // variable, so last-write-wins. Use @-target for per-iteration values. const results = container.querySelectorAll('.result'); expect(results).toHaveLength(2); expect(results[0].textContent).toMatch(/^a-(ok|err)$/); expect(results[1].textContent).toMatch(/^b-(ok|err)$/); }); + + it('computed with @-target writes to local scope per-iteration', () => { + useStoryStore + .getState() + .setTemporary('items', [{ name: 'a', status: 'ok' }]); + + const container = document.createElement('div'); + const passage = makePassage( + 1, + 'Test', + "{for @item of _items}{computed @cls = @item.status == 'ok' ? 'green' : 'red'}{@item.name}-{@cls}{/for}", + ); + act(() => { + render(, container); + }); + + const results = container.querySelectorAll('.result'); + expect(results).toHaveLength(1); + expect(results[0].textContent).toBe('a-green'); + }); + + it('computed @-target produces per-iteration values in multi-item for-loop (#140)', () => { + useStoryStore.getState().setTemporary('items', [ + { name: 'a', status: 'ok' }, + { name: 'b', status: 'err' }, + ]); + + const container = document.createElement('div'); + const passage = makePassage( + 1, + 'Test', + "{for @item of _items}{computed @cls = @item.status == 'ok' ? 'green' : 'red'}{@item.name}-{@cls}{/for}", + ); + act(() => { + render(, container); + }); + + const results = container.querySelectorAll('.result'); + expect(results).toHaveLength(2); + expect(results[0].textContent).toBe('a-green'); + expect(results[1].textContent).toBe('b-red'); + }); + + it('computed @-target outside local scope logs error', () => { + const spy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const container = document.createElement('div'); + const passage = makePassage(1, 'Test', "{computed @cls = 'foo'}"); + act(() => { + render(, container); + }); + + expect(spy).toHaveBeenCalledWith( + expect.stringContaining('{computed @cls'), + expect.any(Error), + ); + spy.mockRestore(); + }); }); describe('{do}', () => {