ReviewOS

stacks/stx

perf: :for does full DOM teardown/rebuild instead of key-based diffing

#1666
Closed glennmichael123 opened this 11 days ago ·
11 days ago

Problem

:for re-renders every item in the loop when any signal in the component scope changes — even signals completely unrelated to the loop data. This causes:

  1. Visual glitch: all :if elements inside the loop flash as they're torn down and re-created (the server-rendered HTML is briefly visible before :if removes it)
  2. Console spam: hundreds of [stx] bindIf REMOVING element log lines per interaction
  3. Performance: O(n) DOM operations for a O(1) state change

Reproduction

<script client lang="ts">
  const items = state([
    { id: '1', title: 'Item A', type: 'run' },
    { id: '2', title: 'Item B', type: 'strength' },
    { id: '3', title: 'Item C', type: 'run' },
  ])

  // This signal is NOT used in the :for loop
  const modalOpen = state(false)
</script>

<div :for="item in items" :key="item.id">
  <span :if="item.type === 'run'">��</span>
  <span :if="item.type === 'strength'">��️</span>
  <span :text="item.title"></span>
</div>

<!-- Clicking this rebuilds ALL 3 loop items -->
<button @click="modalOpen = !modalOpen">Toggle Modal</button>
<div :show="modalOpen">Modal content</div>

Clicking "Toggle Modal" triggers a full teardown/rebuild of all 3 :for items, even though modalOpen has nothing to do with items. Every :if inside the loop fires bindIf REMOVING + bindIf REMOVED, and the elements flash briefly before being re-created.

In a real-world calendar view with 30+ workout cards (each containing ~13 :if directives), a single menu toggle produces 390+ bindIf log lines and a visible UI glitch.

Root Cause

In packages/stx/src/signals.ts (line ~1856), bindFor's effect() callback:

effect(() => {
  currentElements.forEach(e => e.remove());  // tears down ALL items
  currentElements = [];
  // ... re-evaluates list, re-creates all items from scratch
});

This effect subscribes to every signal accessed during evaluation, including signals read by other bindings in the same component scope. When any signal in the scope changes, the effect re-runs and does a full DOM teardown/rebuild.

The :key attribute is stripped during setup (line ~1790) and never used for diffing:

wrapper.removeAttribute(':key');

Two separate issues

Issue 1: Over-broad dependency tracking The :for effect should only re-run when the list signal itself changes, not when unrelated signals (modalOpen, menuOpen, etc.) change. Currently, the effect function runs inside a global effect() that tracks all signal reads in the scope.

Issue 2: No key-based diffing When the list does change, the loop should diff by :key and only update/add/remove the specific items that changed — not tear down and rebuild everything. This is how v-for + :key works in Vue, {#each} + (key) in Svelte, and key prop in React.

Expected Behavior

  1. Changing modalOpen should not trigger any :for re-render
  2. Changing one item in items should only re-render that item, not all items
  3. Adding/removing an item should only add/remove that DOM node, keeping siblings intact
  4. :if elements inside the loop should not flash/glitch on unrelated state changes

Impact

This makes :for impractical for any list with conditional rendering (:if) inside loop items. Common patterns like dropdown menus, accordion toggles, or inline editing inside a list all trigger the full rebuild storm. The workaround (switching :if to :show) hides the visual glitch but the DOM rebuild still happens underneath.

Suggested Approach

  1. Narrow dependency tracking: Evaluate the list expression in an isolated tracking scope so only the list signal is tracked, not every signal in the component
  2. Key-based reconciliation: Use :key to identify items across renders, then diff old vs new arrays and only add/remove/reorder the changed DOM nodes (similar to Vue's patchKeyedChildren)

Environment

  • stx version: 0.2.10 (local linked)
  • Browser: Chrome (latest)
  • Template: coaches/calendar.stx in a training dashboard app (7 days × 3-5 items per day = ~30 cards, each with ~13 :if directives)

🤖 Generated with Claude Code

11 days ago

Fixed on main. bindFor in packages/stx/src/signals.ts:1930 was rewritten:

  1. Narrow dependency tracking — evalLazy (line 2042) uses with(__scope__) { return ... } so only the variables actually accessed by the list expression are tracked. Unrelated signals like modalOpen no longer trigger rebuilds.
  2. Key-based reconciliation — :key is captured at line 1992 and the effect body (lines 2199-2247) builds an oldKeyMap / newKeys diff. Existing DOM nodes are reused via parent.insertBefore instead of remove() + recreate.

Net result: clicking an unrelated modal-toggle button no longer rebuilds the list, and changing one item only touches that item's DOM.

Sign in to comment on this issue.