diff --git a/docs/superpowers/plans/2026-03-28-renderNodes-fast-path.md b/docs/superpowers/plans/2026-03-28-renderNodes-fast-path.md new file mode 100644 index 0000000..82aa836 --- /dev/null +++ b/docs/superpowers/plans/2026-03-28-renderNodes-fast-path.md @@ -0,0 +1,250 @@ +# renderNodes Whitespace-Only Fast Path 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:** Eliminate the micromark + innerHTML pipeline for `{for}` loop iterations whose text nodes are whitespace-only, fixing the 1.5s+ re-render bottleneck in issue #143. + +**Architecture:** Single predicate change in `renderNodes()` — extend the existing `!hasText` fast path to also skip the markdown pipeline when all text nodes contain only whitespace. No new files, no new dependencies. + +**Tech Stack:** Preact, TypeScript, Vitest (happy-dom environment) + +--- + +### Task 1: Write correctness baseline — fast path for HTML + macros with whitespace text + +**Files:** + +- Modify: `test/dom/render.test.tsx` + +This test verifies that a for-loop body containing HTML elements, macros, variables, and only whitespace text nodes renders correctly. The tokenizer produces whitespace text nodes from indentation/newlines between HTML tags — this is the exact pattern from the profiled scenario. + +- [ ] **Step 1: Write the test** + +Add a new `describe` block at the end of the top-level `describe('renderNodes', ...)` in `test/dom/render.test.tsx`: + +```tsx +describe('whitespace-only fast path (issue #143)', () => { + it('renders HTML + macros with only whitespace text nodes', () => { + useStoryStore.getState().setVariable('items', [ + { id: 'a', name: 'Alpha', status: 'active' }, + { id: 'b', name: 'Beta', status: 'locked' }, + { id: 'c', name: 'Gamma', status: 'active' }, + ]); + + // For-loop body is HTML + macros + variables with whitespace between tags. + // The tokenizer produces text nodes for "\n " indentation — these are + // whitespace-only and should not trigger the markdown pipeline. + const markup = [ + '{for @item of $items}', + '
', + ' {@item.name}', + ' {if @item.status === "active"}', + ' Active', + ' {/if}', + '
', + '{/for}', + ].join('\n'); + + const container = document.createElement('div'); + const tokens = tokenize(markup); + const ast = buildAST(tokens); + render(<>{renderNodes(ast)}, container); + + const cards = container.querySelectorAll('.card'); + expect(cards).toHaveLength(3); + expect(cards[0].querySelector('.name')!.textContent).toBe('Alpha'); + expect(cards[0].querySelector('.badge')!.textContent).toBe('Active'); + expect(cards[1].querySelector('.badge')).toBeNull(); + expect(cards[2].querySelector('.badge')!.textContent).toBe('Active'); + }); +}); +``` + +- [ ] **Step 2: Run test to verify it passes (baseline)** + +This test should already pass against the current code since it tests correctness, not the fast path itself. Run it to confirm the test setup is valid: + +Run: `PATH="/home/clemens/.nvm/versions/node/v22.18.0/bin:$PATH" npx vitest run test/dom/render.test.tsx -t "renders HTML + macros with only whitespace text nodes"` +Expected: PASS + +- [ ] **Step 3: Commit** + +```bash +git add test/dom/render.test.tsx +git commit -m "test: add fast path correctness test for HTML + whitespace-only text (#143)" +``` + +--- + +### Task 2: Write correctness baseline — markdown still processed when text has content + +**Files:** + +- Modify: `test/dom/render.test.tsx` + +This test verifies that real markdown syntax inside a for-loop body still goes through micromark. The predicate change must not break this case. + +- [ ] **Step 1: Write the test** + +Add inside the `describe('whitespace-only fast path (issue #143)', ...)` block: + +```tsx +it('preserves markdown processing when text nodes have content', () => { + useStoryStore + .getState() + .setVariable('items', [{ name: 'Alpha' }, { name: 'Beta' }]); + + // Text node "**bold** " has non-whitespace content → must go through micromark + const markup = [ + '{for @item of $items}', + '**{@item.name}** is ready', + '{/for}', + ].join('\n'); + + const container = document.createElement('div'); + const tokens = tokenize(markup); + const ast = buildAST(tokens); + render(<>{renderNodes(ast)}, container); + + const strongs = container.querySelectorAll('strong'); + expect(strongs).toHaveLength(2); + expect(strongs[0].textContent).toBe('Alpha'); + expect(strongs[1].textContent).toBe('Beta'); +}); +``` + +- [ ] **Step 2: Run test to verify it passes (baseline)** + +Run: `PATH="/home/clemens/.nvm/versions/node/v22.18.0/bin:$PATH" npx vitest run test/dom/render.test.tsx -t "preserves markdown processing when text nodes have content"` +Expected: PASS + +- [ ] **Step 3: Commit** + +```bash +git add test/dom/render.test.tsx +git commit -m "test: add markdown preservation test for non-whitespace text (#143)" +``` + +--- + +### Task 3: Write correctness baseline — mixed whitespace and non-whitespace text + +**Files:** + +- Modify: `test/dom/render.test.tsx` + +Edge case: a node list where some text nodes are whitespace-only but at least one has real content. The pipeline must still run. + +- [ ] **Step 1: Write the test** + +Add inside the `describe('whitespace-only fast path (issue #143)', ...)` block: + +```tsx +it('uses markdown pipeline when any text node has non-whitespace content', () => { + // Mix of whitespace text nodes (from indentation) and a real text node + const markup = '
\n A **bold** label\n
'; + + const container = document.createElement('div'); + const tokens = tokenize(markup); + const ast = buildAST(tokens); + render(<>{renderNodes(ast)}, container); + + const box = container.querySelector('.box'); + expect(box).not.toBeNull(); + const strong = box!.querySelector('strong'); + expect(strong).not.toBeNull(); + expect(strong!.textContent).toBe('bold'); +}); +``` + +- [ ] **Step 2: Run test to verify it passes (baseline)** + +Run: `PATH="/home/clemens/.nvm/versions/node/v22.18.0/bin:$PATH" npx vitest run test/dom/render.test.tsx -t "uses markdown pipeline when any text node has non-whitespace content"` +Expected: PASS + +- [ ] **Step 3: Commit** + +```bash +git add test/dom/render.test.tsx +git commit -m "test: add mixed whitespace/content text edge case test (#143)" +``` + +--- + +### Task 4: Implement the fast path predicate change + +**Files:** + +- Modify: `src/markup/render.tsx:274-278` + +- [ ] **Step 1: Modify the fast path check** + +In `src/markup/render.tsx`, replace lines 274-278: + +```tsx +// If there's no text at all, render nodes directly without markdown +const hasText = nodes.some((n) => n.type === 'text'); +if (!hasText) { + return nodes.map((node, i) => renderSingleNode(node, i)); +} +``` + +With: + +```tsx +// Skip the markdown pipeline when text nodes contain only whitespace. +// This eliminates ~97 redundant micromark + innerHTML calls per render +// in {for} loops over HTML + macro content (issue #143). +const needsMarkdown = nodes.some( + (n) => n.type === 'text' && n.value.trim() !== '', +); +if (!needsMarkdown) { + return nodes.map((node, i) => renderSingleNode(node, i)); +} +``` + +- [ ] **Step 2: Run all tests to verify nothing breaks** + +Run: `PATH="/home/clemens/.nvm/versions/node/v22.18.0/bin:$PATH" npx vitest run` +Expected: All tests PASS — the three new tests from Tasks 1-3 plus all existing render, expression, and DOM tests. + +- [ ] **Step 3: Run type check** + +Run: `PATH="/home/clemens/.nvm/versions/node/v22.18.0/bin:$PATH" npx tsc --noEmit` +Expected: No errors + +- [ ] **Step 4: Commit** + +```bash +git add src/markup/render.tsx +git commit -m "fix: skip markdown pipeline for whitespace-only text in renderNodes (#143) + +Extend the existing fast path to detect when all text nodes contain only +whitespace (indentation, newlines between HTML tags). In this case, skip +the micromark + innerHTML + DOM walk pipeline entirely and render nodes +directly via renderSingleNode. + +For a {for} loop over 23 HTML cards, this eliminates ~97 redundant +pipeline calls that account for 89% of wall time in Chrome profiles." +``` + +--- + +### Task 5: Run full test suite and verify + +**Files:** None (verification only) + +- [ ] **Step 1: Run the complete test suite** + +Run: `PATH="/home/clemens/.nvm/versions/node/v22.18.0/bin:$PATH" npx vitest run` +Expected: All tests PASS + +- [ ] **Step 2: Run type check** + +Run: `PATH="/home/clemens/.nvm/versions/node/v22.18.0/bin:$PATH" npx tsc --noEmit` +Expected: No errors + +- [ ] **Step 3: Verify the fast path test specifically** + +Run: `PATH="/home/clemens/.nvm/versions/node/v22.18.0/bin:$PATH" npx vitest run test/dom/render.test.tsx -t "whitespace-only fast path"` +Expected: All 3 tests in the `whitespace-only fast path` describe block PASS diff --git a/docs/superpowers/specs/2026-03-28-renderNodes-fast-path-design.md b/docs/superpowers/specs/2026-03-28-renderNodes-fast-path-design.md new file mode 100644 index 0000000..ab9ab0c --- /dev/null +++ b/docs/superpowers/specs/2026-03-28-renderNodes-fast-path-design.md @@ -0,0 +1,72 @@ +# Design: renderNodes whitespace-only fast path + +**Issue:** #143 — Performance: renderNodes runs full micromark + innerHTML pipeline per {for} iteration +**Date:** 2026-03-28 + +## Problem + +Passages with `{for}` loops over moderate-sized arrays (15-25 items) take 1.5-1.7 seconds to re-render. Chrome CPU profiles show 89% of wall time inside `htmlToPreact` (`src/markup/render.tsx:38`), called once per `{for}` iteration via `renderNodes`. + +Each call runs the full pipeline: build markdown string with placeholders → run micromark (CommonMark + GFM) → `createElement('div')` + `innerHTML` (browser HTML parser) → nobr `

` stripping → recursive DOM walk → Preact vnodes. + +For a passage with a `{for}` loop over 23 items containing HTML elements and `{computed}` expressions, this pipeline runs ~97 times per render. For loop bodies that are 100% HTML + Spindle macros with only whitespace as text, the entire micromark + innerHTML path is pure overhead — micromark wraps content in `

` tags that nobr immediately strips, and innerHTML creates DOM nodes that are immediately discarded after vnode extraction. + +## Solution + +Extend the existing fast path in `renderNodes` to detect whitespace-only text nodes and skip the markdown pipeline when no meaningful markdown content is present. + +### Current fast path (`render.tsx:274-278`) + +```ts +const hasText = nodes.some((n) => n.type === 'text'); +if (!hasText) { + return nodes.map((node, i) => renderSingleNode(node, i)); +} +``` + +This skips the pipeline when there are zero text nodes. But in practice, for-loop bodies almost always contain whitespace text nodes (indentation, newlines between HTML tags), so the fast path never triggers. + +### New fast path + +```ts +const needsMarkdown = nodes.some( + (n) => n.type === 'text' && n.value.trim() !== '', +); +if (!needsMarkdown) { + return nodes.map((node, i) => renderSingleNode(node, i)); +} +``` + +If all text nodes contain only whitespace, skip the pipeline and render each node directly via `renderSingleNode`. Text nodes return their value as-is (whitespace), and non-text nodes (html, macro, variable, expression) render through their existing component paths. + +### Why this is safe + +Whitespace-only text between HTML tags and macros cannot produce meaningful markdown output: + +- **nobr mode:** Micromark wraps whitespace in `

` tags that nobr immediately strips. Skipping produces identical output. +- **non-nobr mode:** Whitespace-only `

` tags are empty and invisible. The rendered output is identical whether we run the pipeline or skip it. +- **Paragraph breaks:** Blank lines in whitespace-only text between non-text nodes (HTML elements, macros) don't create visible paragraph separation — those nodes render as their own components independent of `

` wrapping. + +### What this eliminates + +For a 23-item for-loop with HTML cards (the profiled scenario): + +- ~97 micromark parse calls +- ~97 `createElement('div')` + `innerHTML` DOM constructions +- ~97 nobr `querySelectorAll(':scope > p')` + `replaceWith` passes +- ~97 recursive DOM tree walks to build Preact vnodes +- Associated GC pressure (18-38 MB heap churn from temporary DOM nodes) + +## Files changed + +| File | Change | +| -------------------------- | ------------------------------------------------- | +| `src/markup/render.tsx` | Modify the `needsMarkdown` check in `renderNodes` | +| `test/dom/render.test.tsx` | Add tests for fast path behavior | + +## Test plan + +1. **Fast path triggers for HTML+macros with whitespace text** — for-loop body with HTML elements, macros, variables, and only whitespace text nodes renders correctly +2. **Slow path preserved for real markdown** — for-loop body with markdown syntax (`**bold**`, lists, tables) still processes through micromark +3. **Edge case: mixed nodes** — whitespace text + non-whitespace text in the same node list goes through the pipeline +4. **All existing render tests pass** — ~50 tests covering markdown, variables, links, tables, SVG, nobr, className/id, computed, consecutive mutations diff --git a/src/markup/render.tsx b/src/markup/render.tsx index 31f725b..6078490 100644 --- a/src/markup/render.tsx +++ b/src/markup/render.tsx @@ -271,9 +271,16 @@ export function renderNodes( ): preact.ComponentChildren { if (nodes.length === 0) return null; - // If there's no text at all, render nodes directly without markdown - const hasText = nodes.some((n) => n.type === 'text'); - if (!hasText) { + // Skip the markdown pipeline when text nodes contain only whitespace. + // This eliminates ~97 redundant micromark + innerHTML calls per render + // in {for} loops over HTML + macro content (issue #143). + // However, don't skip if any text node contains blank lines (\n\n), as those + // have markdown semantics (paragraph separation). + const needsMarkdown = nodes.some( + (n) => + n.type === 'text' && (n.value.trim() !== '' || /\n\s*\n/.test(n.value)), + ); + if (!needsMarkdown) { return nodes.map((node, i) => renderSingleNode(node, i)); } diff --git a/test/dom/render.test.tsx b/test/dom/render.test.tsx index 925bb46..9b7a65a 100644 --- a/test/dom/render.test.tsx +++ b/test/dom/render.test.tsx @@ -465,4 +465,100 @@ describe('renderNodes', () => { expect(el.innerHTML).not.toContain('<svg'); }); }); + + // Issue #136: consecutive {set} macros can't see each other's temp mutations + describe('consecutive {set} mutations (issue #136)', () => { + it('second {set} sees temp set by first {set}', () => { + const container = document.createElement('div'); + const tokens = tokenize( + '{set _x = [3, 1, 2]}{set _y = _x.slice().sort()}Result: {_y}', + ); + const ast = buildAST(tokens); + render(<>{renderNodes(ast)}, container); + expect(container.textContent).toContain('Result: 1,2,3'); + }); + + it('second {set} sees $var set by first {set}', () => { + const container = document.createElement('div'); + const tokens = tokenize('{set $a = 10}{set $b = $a + 5}Answer: {$b}'); + const ast = buildAST(tokens); + render(<>{renderNodes(ast)}, container); + expect(container.textContent).toContain('Answer: 15'); + }); + }); + + describe('whitespace-only fast path (issue #143)', () => { + it('renders HTML + macros with only whitespace text nodes', () => { + useStoryStore.getState().setVariable('items', [ + { id: 'a', name: 'Alpha', status: 'active' }, + { id: 'b', name: 'Beta', status: 'locked' }, + { id: 'c', name: 'Gamma', status: 'active' }, + ]); + + // For-loop body is HTML + macros + variables with whitespace between tags. + // The tokenizer produces text nodes for "\n " indentation — these are + // whitespace-only and should not trigger the markdown pipeline. + const markup = [ + '{for @item of $items}', + '

', + ' {@item.name}', + ' {if @item.status === "active"}', + ' Active', + ' {/if}', + '
', + '{/for}', + ].join('\n'); + + const container = document.createElement('div'); + const tokens = tokenize(markup); + const ast = buildAST(tokens); + render(<>{renderNodes(ast)}, container); + + const cards = container.querySelectorAll('.card'); + expect(cards).toHaveLength(3); + expect(cards[0].querySelector('.name')!.textContent).toBe('Alpha'); + expect(cards[0].querySelector('.badge')!.textContent).toBe('Active'); + expect(cards[1].querySelector('.badge')).toBeNull(); + expect(cards[2].querySelector('.badge')!.textContent).toBe('Active'); + }); + + it('preserves markdown processing when text nodes have content', () => { + useStoryStore + .getState() + .setVariable('items', [{ name: 'Alpha' }, { name: 'Beta' }]); + + // Text node "**bold** " has non-whitespace content → must go through micromark + const markup = [ + '{for @item of $items}', + '**{@item.name}** is ready', + '{/for}', + ].join('\n'); + + const container = document.createElement('div'); + const tokens = tokenize(markup); + const ast = buildAST(tokens); + render(<>{renderNodes(ast)}, container); + + const strongs = container.querySelectorAll('strong'); + expect(strongs).toHaveLength(2); + expect(strongs[0].textContent).toBe('Alpha'); + expect(strongs[1].textContent).toBe('Beta'); + }); + + it('uses markdown pipeline when any text node has non-whitespace content', () => { + // Mix of whitespace text nodes (from indentation) and a real text node + const markup = '
\n A **bold** label\n
'; + + const container = document.createElement('div'); + const tokens = tokenize(markup); + const ast = buildAST(tokens); + render(<>{renderNodes(ast)}, container); + + const box = container.querySelector('.box'); + expect(box).not.toBeNull(); + const strong = box!.querySelector('strong'); + expect(strong).not.toBeNull(); + expect(strong!.textContent).toBe('bold'); + }); + }); });