fix: a mid-commit throw strands a row in a plain .map() array

.agents/skills/webjs/references/components.md

@@ -181,7 +181,9 @@ Errors are isolated per component by default (no user code): a thrown `await` re
 
 The boundary also covers `watch(signal)` (its notify microtask) and `until()` (its promise resolution), which commit outside the update cycle. A throw from either used to surface at the window instead of the owning component. It routes to the component whose TEMPLATE holds the binding, which is not always the element the binding sits inside: `html`<child-el>${watch(sig)}</child-el>`` belongs to the parent that wrote it, not to `child-el`. The `asyncAppend` / `asyncReplace` path is NOT covered, in two distinct ways: its own iteration throw is swallowed to `console.error` on purpose (an author's iterable should handle it), and a `watch` / `until` nested inside a chunk it commits is installed with no owner in scope, so that one still reaches the window.
 
-**A commit that throws leaves the directive's own state consistent, so the NEXT valid render is correct.** (One reconciler is still exempt: a plain `.map()` array whose item TEMPLATE SHAPE changes in the same render that throws can strand a row. `repeat()` is the keyed path and is repaired.) This matters because the corruption is otherwise silent: the renders that expose it are fully valid and log nothing after the first throw. The hole whose commit threw is marked so the next render re-applies it rather than skipping it as unchanged (its recorded value is never advanced past a throw, and would otherwise match exactly what the recovering render supplies, leaving a child region blank for good). `repeat()` additionally repairs its key map so the map describes the DOM again, and the next render is an ordinary reconcile that repositions every row (the failure was a permanently duplicated row); it is deliberately NOT a rebuild of the region, which would discard the node identity keyed reconciliation exists to preserve. `guard()` records its new deps only once the commit succeeds, so a later render with those same deps re-renders the region instead of short-circuiting past a region the throw had blanked; `until()` advances its resolved priority only after the commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it.
+**A commit that throws leaves the directive's own state consistent, so the NEXT valid render is correct.** This matters because the corruption is otherwise silent: the renders that expose it are fully valid and log nothing after the first throw. The hole whose commit threw is marked so the next render re-applies it rather than skipping it as unchanged (its recorded value is never advanced past a throw, and would otherwise match exactly what the recovering render supplies, leaving a child region blank for good). Both list reconcilers additionally repair their own bookkeeping so it describes the DOM again, and the next render is an ordinary reconcile rather than a rebuild of the region, which would discard the node identity the reconcilers exist to preserve. `repeat()` re-unites its key map and repositions every row (the failure was a permanently duplicated row). A plain `.map()` array splices the part of its slot list the failed pass never reached back on, which matters whenever a slot is REPLACED rather than updated in place (its template shape changed, its kind changed between text, template and empty, or the array grew past its old length), since that is the branch that inserts the replacement before removing what it replaced (the failure was a stranded row that outlived even a render of an empty array). `guard()` records its new deps only once the commit succeeds, so a later render with those same deps re-renders the region instead of short-circuiting past a region the throw had blanked; `until()` advances its resolved priority only after the commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it.
+
+**Teardown is total as well.** Removing a row is not a commit and has no retry, so a throw while tearing one down cannot be allowed to abandon the rest. Unbinding a `ref` during teardown can never abort the removal of the remaining rows, and `repeat()` drops each leftover key from its map before touching that row, so the map never describes a row that has already been removed (which used to leave the row the app DELETED on screen, reorder the survivors, and let a later render that re-added that key reinsert the disposed instance). To make that hold, a `ref` whose object `value` setter throws is now SWALLOWED on teardown, matching the ref CALLBACK, which was already swallowed everywhere. That is a deliberate divergence from lit, which guards neither and propagates from both. It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches `renderError()`, because there the boundary can report it and the next render can repair it.
 
 Decision rules. Use `async render()` for request-time server data that should be in the first paint (the default). Add `renderFallback()` when a client re-fetch's stale content would mislead. Use `Task` / signals for genuinely client-only data (a click, viewport, live updates). For SLOW data where blocking the first byte hurts, wrap the region in `<webjs-suspense .fallback=${html\`Loading...\`}>` to stream it (the only way to show a first-paint fallback; see `client-router-and-streaming.md`). Do NOT fetch in `connectedCallback` for data knowable server-side, and do NOT prop-drill what a leaf can fetch itself.
 

packages/core/src/render-client.js

@@ -1078,12 +1078,19 @@ function clearInstance(inst, container) {
   for (const p of inst.bound) {
     if (p.kind === 'event') p.el.removeEventListener(p.name, p.dispatcher);
     if (p.kind === 'element') {
+      // Guarded for the same reason as the sibling in `disposeInstance`, and
+      // the stakes are higher here: this is the container-level teardown
+      // `render()` runs before a template swap, so a throw skips the rest of
+      // this loop AND the `replaceChildren()` below, leaving the old DOM in
+      // place with `host[INSTANCE]` never reassigned. Since `lastTarget` is
+      // cleared only after the write, every later swap of that container
+      // then throws at the same part, permanently.
       const prev = /** @type any */ (p).lastTarget;
       if (prev) {
         if (typeof prev === 'function') {
           try { prev(undefined); } catch { /* swallow */ }
         } else if (typeof prev === 'object') {
-          prev.value = undefined;
+          try { prev.value = undefined; } catch { /* swallow */ }
         }
         /** @type any */ (p).lastTarget = undefined;
         /** @type any */ (p).__lastEl = undefined;
@@ -1756,12 +1763,33 @@ function disposeInstance(inst) {
       // Unbind any active ref so the user observes the element being
       // removed (callback receives undefined / Ref.value cleared).
       // Mirrors lit-html's cleanup-on-disconnect for element parts.
+      //
+      // BOTH branches swallow, and lit is not the reason: lit's ref directive
+      // guards neither, so a throw there propagates. The reason is that a
+      // teardown has to be TOTAL. `lastTarget` is cleared only AFTER these
+      // writes, so a throw leaves the part still pointing at the ref and
+      // every later teardown of the same instance throws at the same line
+      // forever. It also aborts the rest of this loop, so the remaining
+      // parts keep their listeners and their refs bound. A teardown has no
+      // retry either (a commit has the COMMIT_FAILED sentinel and a next
+      // render; this does not), so there is nothing a propagated error could
+      // usefully repair.
+      //
+      // The object branch is the one this adds. The callback branch was
+      // already guarded here AND on the commit path (`applyElement` wraps
+      // every `nextTarget(...)` / `prevTarget(undefined)` call), so a
+      // throwing ref CALLBACK has always been swallowed everywhere. What was
+      // inconsistent is the object ref, guarded on neither. This makes the
+      // two agree on TEARDOWN, which is where the totality argument bites.
+      // It does NOT touch the commit path, so `applyElement`'s object-ref
+      // writes still propagate to the component boundary, which has a route
+      // for the error and a next render to repair it.
       const prev = /** @type any */ (p).lastTarget;
       if (prev) {
         if (typeof prev === 'function') {
           try { prev(undefined); } catch { /* swallow */ }
         } else if (typeof prev === 'object') {
-          prev.value = undefined;
+          try { prev.value = undefined; } catch { /* swallow */ }
         }
         /** @type any */ (p).lastTarget = undefined;
         /** @type any */ (p).__lastEl = undefined;
@@ -1828,20 +1856,36 @@ function reconcileRepeat(part, value) {
         state.map.delete(key);
       } else {
         if (existing) {
+          // Unmapped BEFORE the row is touched, for the reason spelled out
+          // in the leftover loop below: a key kept across a refused removal
+          // points at a half-removed row, and reusing that row later walks
+          // the removal off the end of the region. Same ordering, same
+          // trade, and the two have to agree or the invariant the catch
+          // relies on holds on one branch and not the other.
+          state.map.delete(key);
           disposeInstance(existing);
           removeBetween(existing.startNode, existing.endNode);
-          state.map.delete(key);
         }
         const { inst, frag } = buildDetached(/** @type any */ (tr));
         parent.insertBefore(frag, marker);
         newMap.set(key, inst);
       }
     }
 
-    // Remove any keys that remain in the old map.
-    for (const inst of state.map.values()) {
-      disposeInstance(inst);
-      removeBetween(inst.startNode, inst.endNode);
+    // Remove any keys that remain in the old map. The key leaves the map
+    // BEFORE its row is touched and the removal is in a `finally`, so at any
+    // throw point `state.map` holds exactly the leftovers this pass has not
+    // reached, and a row whose dispose threw still leaves the document.
+    // Iterating a snapshot keeps the delete obviously safe rather than
+    // relying on the reader knowing that deleting during a Map iteration is
+    // legal.
+    for (const [k, inst] of [...state.map]) {
+      state.map.delete(k);
+      try {
+        disposeInstance(inst);
+      } finally {
+        removeBetween(inst.startNode, inst.endNode);
+      }
     }
     state.map = newMap;
   } catch (err) {
@@ -1861,6 +1905,33 @@ function reconcileRepeat(part, value) {
     // reconcile against a truthful map, which repositions every row and
     // re-applies whatever the throw skipped.
     //
+    // That claim covers the REMOVAL loop as well as the walk, and only
+    // because the loop was written to earn it. It drops each key before
+    // touching that row and removes the nodes in a `finally`, so a throw
+    // mid-removal cannot merge `newMap` over a `state.map` still holding
+    // disposed, detached rows. That was the failure: the row the app DELETED
+    // stayed on screen, the survivors reordered, and a later render that
+    // re-added that key reinserted the detached instance. The invariant, at
+    // any throw point on either branch: every instance this pass has not
+    // destructively touched is described by exactly one of the two maps,
+    // `newMap` for the processed new keys and `state.map` for the leftovers
+    // not reached yet, which is what makes the merge below correct. The
+    // exception is the row named in the residual just below, whose removal
+    // refused part-way; that one is in neither map, by choice.
+    //
+    // The residual is a throw from `removeBetween` ITSELF, which only calls
+    // `removeChild` on nodes the renderer owns, so it takes a throwing DOM to
+    // reach. That row is already unmapped, so its remaining nodes stay in the
+    // document tracked by nothing and a later re-add of that key builds a
+    // second row beside them. Unmapping AFTER the removal instead would keep
+    // that key, and it is measurably worse rather than better: the row is
+    // half removed, its start marker gone and its end marker still in place,
+    // so the re-add hits the reuse branch and `moveRange` re-attaches the
+    // lone start marker AFTER the end marker. The next removal of that key
+    // then walks forward from a start that never reaches its end, taking the
+    // repeat part's own marker and every following sibling with it, and the
+    // region is dead for good. One untracked row beats a destroyed list.
+    //
     // Deliberately NOT a teardown-and-rebuild of the region. Rebuilding is
     // the obvious defensive move and it is measurably worse: it discards node
     // identity for every row, which is the exact cost keyed reconciliation
@@ -1884,9 +1955,16 @@ function reconcileRepeat(part, value) {
 
 /** @param {{ kind: 'repeat', map: Map<any, TemplateInstance> }} state */
 function teardownRepeat(state) {
-  for (const inst of state.map.values()) {
-    disposeInstance(inst);
-    removeBetween(inst.startNode, inst.endNode);
+  // Same delete-as-you-go shape as the leftover loop in `reconcileRepeat`,
+  // for the same reason: a throw part-way must not leave already-removed
+  // instances in the map. The trailing `clear()` stays as a no-op safety net.
+  for (const [k, inst] of [...state.map]) {
+    state.map.delete(k);
+    try {
+      disposeInstance(inst);
+    } finally {
+      removeBetween(inst.startNode, inst.endNode);
+    }
   }
   state.map.clear();
 }
@@ -1977,42 +2055,107 @@ function reconcileArray(part, value) {
   const old = state.items;
   /** @type {ArrayItem[]} */
   const next = [];
+  // How many slots of `old` are fully processed. Tracked rather than inferred
+  // from `next.length`, because the shrink loop below advances through `old`
+  // while `next` stops growing, so the two part company there. The catch is
+  // the only reader.
+  let consumed = 0;
 
-  for (let i = 0; i < value.length; i++) {
-    const v = value[i];
-    const o = old[i];
-    if (isTemplate(v)) {
-      const tr = /** @type any */ (v);
-      if (o && o.type === 'tpl' && o.inst.strings === tr.strings) {
-        updateInstance(o.inst, tr.values);
-        next.push(o);
-        continue;
-      }
-    } else if (v != null && v !== false && v !== true) {
-      if (o && o.type === 'text') {
-        const str = String(v);
-        if (o.node.data !== str) o.node.data = str;
-        next.push(o);
+  try {
+    for (let i = 0; i < value.length; i++) {
+      const v = value[i];
+      const o = old[i];
+      if (isTemplate(v)) {
+        const tr = /** @type any */ (v);
+        if (o && o.type === 'tpl' && o.inst.strings === tr.strings) {
+          updateInstance(o.inst, tr.values);
+          next.push(o);
+          consumed = i + 1;
+          continue;
+        }
+      } else if (v != null && v !== false && v !== true) {
+        if (o && o.type === 'text') {
+          const str = String(v);
+          if (o.node.data !== str) o.node.data = str;
+          next.push(o);
+          consumed = i + 1;
+          continue;
+        }
+      } else {
+        // Empty slot: drop any prior nodes that occupied this position.
+        // Deliberately NOT reordered like the branch below. That reorder
+        // exists to keep a slot that was already BUILT and INSERTED tracked,
+        // and this branch builds and inserts nothing, so pushing first would
+        // only mean a throw from the removal leaves a phantom empty slot at
+        // this index AND `old[i]` spliced in at the next one, shifting every
+        // later slot by one in a POSITIONAL reconciler. Removing first, a
+        // throw here leaves `old[i]` describing its own position, which is
+        // still exactly where its nodes are.
+        if (o) removeArrayItem(o);
+        next.push({ type: 'empty' });
+        consumed = i + 1;
         continue;
       }
-    } else {
-      // Empty slot: drop any prior nodes that occupied this position.
+      // Shape changed, or the array grew past the old length. Build fresh,
+      // insert at this position (before the current / next still-attached
+      // old node, else the marker), then drop the old slot it replaced.
+      // The push sits BEFORE the removal, which is a pure reordering on the
+      // success path and means a slot that has already been built and
+      // inserted is never untracked at any throw point.
+      const { item, frag } = buildArrayItem(v);
+      if (frag) parent.insertBefore(frag, nextArrayAnchor(old, i, marker));
+      next.push(item);
       if (o) removeArrayItem(o);
-      next.push({ type: 'empty' });
-      continue;
+      consumed = i + 1;
     }
-    // Shape changed, or the array grew past the old length. Build fresh,
-    // insert at this position (before the current / next still-attached
-    // old node, else the marker), then drop the old slot it replaced.
-    const { item, frag } = buildArrayItem(v);
-    if (frag) parent.insertBefore(frag, nextArrayAnchor(old, i, marker));
-    if (o) removeArrayItem(o);
-    next.push(item);
-  }
 
-  // Shrink: remove slots beyond the new length.
-  for (let i = value.length; i < old.length; i++) removeArrayItem(old[i]);
-  state.items = next;
+    // Shrink: remove slots beyond the new length.
+    for (let i = value.length; i < old.length; i++) {
+      removeArrayItem(old[i]);
+      consumed = i + 1;
+    }
+    state.items = next;
+  } catch (err) {
+    // `state.items` is committed only after the whole walk, so a throw part
+    // way through discards `next` entirely: the map of slots keeps describing
+    // positions whose nodes were already removed, while the freshly built and
+    // inserted ones are in the document tracked by nothing. Nothing is logged
+    // after the first throw, and the orphan outlives even a render of an EMPTY
+    // array, because the only code that could remove it walks `state.items`.
+    //
+    // Splice the untouched tail of `old` onto what `next` accumulated, so the
+    // bookkeeping describes the DOM again. The invariant that holds at any
+    // throw point: every live node is described by exactly one slot, the
+    // slots below `next.length` being the rebuilt or reused ones and the rest
+    // the part of `old` this pass never reached. Index alignment survives
+    // because this reconciler is POSITIONAL, so a slot's index IS its
+    // identity, which is also why the boundary has to come from `consumed`
+    // rather than `next.length`: during the shrink loop those differ, and
+    // splicing from `next.length` would re-describe slots already removed.
+    // A later render that grew the array would then match a live value
+    // against a DETACHED slot with the same `strings`, update it in place,
+    // and that row would silently never appear.
+    //
+    // Deliberately NOT a teardown-and-rebuild of the region, for the reason
+    // recorded on `reconcileRepeat`'s catch above: it discards node identity
+    // for every row, which cancels an in-progress native drag and drops focus
+    // and scroll.
+    //
+    // The residual is a throw from the removal step itself, which takes a
+    // throwing DOM to reach (the teardown it calls is total, so only
+    // `removeBetween` is left, and that calls `removeChild` solely on nodes
+    // the renderer owns). State it rather than deny it: `removeBetween`
+    // takes the start marker first and then early-returns for good once that
+    // marker is gone, so a row whose removal refused part-way can never be
+    // removed afterwards. Its remaining nodes stay in the document, and an
+    // EMPTY render will not clear them. Tracked or not, they are there for
+    // the life of the region, the same residual `reconcileRepeat`'s catch
+    // names. What this repair buys is that there is only ONE such row and
+    // every other slot still reconciles, where before the whole pass was
+    // discarded.
+    state.items = next.concat(old.slice(consumed));
+    throw err;
+  }
 }
 
 /**