From 80e47cfcd527567556378e48be0a3f51cbc22016 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 15:42:08 +0530 Subject: [PATCH 1/6] fix: teardown is total, so a repeat() leftover throw cannot strand a row A throw inside reconcileRepeat's leftover-removal loop left rows that were already disposed and detached still registered in state.map. The visible result was the opposite of what the app asked for: the row it removed stayed on screen, the survivors reordered, and a later render that re-added that key reinserted the detached instance. Two changes. The object-ref write in disposeInstance and clearInstance is guarded to match the callback-ref branch beside it, because neither site clears lastTarget before that write, so a throw made every later teardown of the same instance throw again at the same line. In clearInstance that was permanent: the throw skipped the container's replaceChildren(), so the old DOM stayed and every future template swap of that container threw. The removal loop then deletes each key before touching its row and removes the nodes in a finally, so at any throw point state.map holds exactly the leftovers still in the document. That closes the class rather than the one trigger, since removeBetween stays inside the loop. teardownRepeat gets the same shape. Swallowing a throwing ref unbind is a deliberate divergence from lit, which propagates from both branches. A teardown has no retry, and it has to be total or it abandons the parts behind it. The commit path is untouched: a throwing ref there still reaches the component boundary. --- .agents/skills/webjs/references/components.md | 2 + packages/core/src/render-client.js | 72 ++++++++-- .../browser/directive-commit-throw.test.js | 27 +++- .../rendering/directive-commit-throw.test.js | 127 +++++++++++++++++- website/app/docs/error-handling/page.ts | 1 + 5 files changed, 217 insertions(+), 12 deletions(-) diff --git a/.agents/skills/webjs/references/components.md b/.agents/skills/webjs/references/components.md index f4b02559f..77c53664c 100644 --- a/.agents/skills/webjs/references/components.md +++ b/.agents/skills/webjs/references/components.md @@ -183,6 +183,8 @@ The boundary also covers `watch(signal)` (its notify microtask) and `until()` (i **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. +**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 a row is either gone from both the map and the document or still in both, never removed but still mapped (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 callback or whose object `value` setter throws is SWALLOWED on teardown. That is a deliberate divergence from lit, which propagates from both, and it applies only to teardown: on the commit path a throwing `ref` 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 `` 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. ## Task: client-only async data diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index b311dbd3c..e52d889bc 100644 --- a/packages/core/src/render-client.js +++ b/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,28 @@ 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. An object ref whose setter throws and a callback + // ref that throws are the same act by the author, and they get the + // same contract. This is a deliberate divergence from lit, recorded in + // the docs. The COMMIT path is different and stays unguarded (see + // `applyElement`): a commit throw has a route (the component boundary) + // and a repair, so swallowing there would hide a real author error. 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; @@ -1838,10 +1861,19 @@ function reconcileRepeat(part, value) { } } - // 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 still in the + // document, and a row whose dispose threw still leaves it. 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 +1893,21 @@ 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 deletes each key before + // touching its row and removes the nodes in a `finally`, so a leftover is + // either fully gone from both the map and the document or still in both, + // never removed-but-still-mapped. Without that, a throw mid-removal would + // merge `newMap` over a `state.map` still holding disposed, detached + // rows: the row the app DELETED stays on screen, the survivors reorder, + // and a later render that re-adds that key reinserts the detached + // instance. The invariant, at any throw point on either branch: every + // instance still in the document 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. + // One residual: a throw from `removeBetween` itself, which only calls + // `removeChild` on nodes the renderer owns. + // // 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 +1931,16 @@ function reconcileRepeat(part, value) { /** @param {{ kind: 'repeat', map: Map }} 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(); } diff --git a/packages/core/test/rendering/browser/directive-commit-throw.test.js b/packages/core/test/rendering/browser/directive-commit-throw.test.js index a2e7301c4..956f8d459 100644 --- a/packages/core/test/rendering/browser/directive-commit-throw.test.js +++ b/packages/core/test/rendering/browser/directive-commit-throw.test.js @@ -16,7 +16,7 @@ import { html } from '../../../src/html.js'; import { render } from '../../../src/render-client.js'; import { repeat } from '../../../src/repeat.js'; -import { watch } from '../../../src/directives.js'; +import { watch, ref } from '../../../src/directives.js'; import { signal } from '../../../src/signal.js'; import { WebComponent } from '../../../src/component.js'; @@ -215,6 +215,31 @@ suite('directive commit throws (browser)', () => { assert.strictEqual(after[2], before[2]); }); + test('a throwing ref unbind removes the row, and re-adding it builds a new element', () => { + // The identity facts linkedom cannot prove: the survivor is MOVED rather + // than rebuilt, and the resurrected key is a genuinely new element rather + // than the disposed instance handed back. + const boom = { set value(v) { if (v === undefined) throw new Error('ref-boom'); }, get value() { return null; } }; + const refRows = (items) => html`
    ${repeat( + items, + (it) => it.id, + (it) => html`
  • ${it.n}
    • `, + )}
`; + + render(refRows([{ id: 1, n: 'a' }, { id: 9, n: 'doomed' }]), container); + const before = [...container.querySelectorAll('li')]; + + render(refRows([{ id: 1, n: 'a' }]), container); + assert.deepEqual([...container.querySelectorAll('li')].map((li) => li.textContent), ['a']); + assert.strictEqual(container.querySelector('li'), before[0]); + + render(refRows([{ id: 1, n: 'a' }, { id: 9, n: 'again' }]), container); + const after = [...container.querySelectorAll('li')]; + assert.deepEqual(after.map((li) => li.textContent), ['a', 'again']); + assert.strictEqual(after[0], before[0]); + assert.ok(after[1] !== before[1], 'the disposed instance must not be resurrected'); + }); + test('removing rows after recovery leaves nothing behind', () => { render(rows(good), container); throwsMatching(() => { diff --git a/packages/core/test/rendering/directive-commit-throw.test.js b/packages/core/test/rendering/directive-commit-throw.test.js index 546c6b2d3..56b4f6278 100644 --- a/packages/core/test/rendering/directive-commit-throw.test.js +++ b/packages/core/test/rendering/directive-commit-throw.test.js @@ -27,11 +27,11 @@ before(() => { globalThis.HTMLElement = window.HTMLElement; }); -let html, render, guard, until, watch, repeat, signal; +let html, render, guard, until, watch, ref, repeat, signal; before(async () => { ({ html } = await import('../../src/html.js')); ({ render } = await import('../../src/render-client.js')); - ({ guard, until, watch } = await import('../../src/directives.js')); + ({ guard, until, watch, ref } = await import('../../src/directives.js')); ({ repeat } = await import('../../src/repeat.js')); ({ signal } = await import('../../src/signal.js')); }); @@ -39,6 +39,18 @@ before(async () => { /** A value that throws when a commit stringifies it. */ const poison = { toString() { throw new Error('boom'); } }; +/** + * A ref whose object write throws on UNBIND. Every other poison in this file + * throws from a COMMIT, and none of them can reach the teardown paths below: + * a commit stringifies its value on the way into the DOM, while these throws + * come from tearing a row back out, which is a different code path with a + * different repair. Only an object ref (or a callback ref) is called during + * teardown at all, so it is the only way in. + */ +function throwingRef(message) { + return { set value(v) { if (v === undefined) throw new Error(message); }, get value() { return null; } }; +} + /** Text content of the rendered rows, ignoring marker comments. */ function rowTexts(container) { return [...container.querySelectorAll('li')].map((li) => li.textContent); @@ -167,6 +179,117 @@ test('repeat: a throw in a nested-template row recovers', () => { assert.deepEqual([...container.querySelectorAll('b')].map((b) => b.textContent), ['one', 'two']); }); +// --- teardown throws (repeat's leftover-removal loop, and clearInstance) --- + +test('repeat: dropping a row whose ref unbind throws still removes that row', () => { + const container = document.createElement('div'); + const rows = (items) => html`
    ${repeat( + items, + (it) => it.id, + (it) => html`
  • ${it.n}
    • `, + )}
`; + + render(rows([{ id: 1, n: 'a' }, { id: 9, n: 'doomed' }]), container); + assert.deepEqual(rowTexts(container), ['a', 'doomed']); + const beforeA = container.querySelector('li'); + + // The app asked for a one-row list. It used to get a two-row list led by + // the row it deleted, because the unbind threw out of the removal loop. + render(rows([{ id: 1, n: 'a' }]), container); + assert.deepEqual(rowTexts(container), ['a']); + + // And the survivor is the same element, not a rebuild. + assert.equal(container.querySelector('li'), beforeA); + + // Still reconciling normally afterwards, rather than wedged. + render(rows([{ id: 1, n: 'A' }]), container); + assert.deepEqual(rowTexts(container), ['A']); +}); + +test('repeat: re-adding a key dropped through a throwing unbind builds a fresh row', () => { + const container = document.createElement('div'); + const rows = (items) => html`
    ${repeat( + items, + (it) => it.id, + (it) => html`
  • ${it.n}
    • `, + )}
`; + + render(rows([{ id: 1, n: 'a' }, { id: 9, n: 'doomed' }]), container); + const doomed = [...container.querySelectorAll('li')][1]; + + render(rows([{ id: 1, n: 'a' }]), container); + render(rows([{ id: 1, n: 'a' }, { id: 9, n: 'again' }]), container); + + assert.deepEqual(rowTexts(container), ['a', 'again']); + // A disposed, detached instance must never come back: it is unmapped, so + // the key misses and builds fresh. + const readded = [...container.querySelectorAll('li')][1]; + assert.notEqual(readded, doomed); +}); + +test('repeat: a throw INSIDE the removal loop leaves no leftover still mapped', () => { + // Drives the throw from the loop's OTHER step, so this covers the loop's + // shape independently of the ref guard above (which makes the dispose step + // unable to throw at all). Nothing here reaches into module internals: it + // patches the rows' parent so one DOM removal refuses. + const container = document.createElement('div'); + const rows = (items) => html`
    ${repeat( + items, + (it) => it.id, + (it) => html`
  • ${it.n}
    • `, + )}
`; + + render(rows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 3, n: 'three' }]), container); + const [, liTwo, liThree] = [...container.querySelectorAll('li')]; + + const ul = container.querySelector('ul'); + const origRemove = ul.removeChild.bind(ul); + ul.removeChild = (node) => { + if (node === liThree) throw new Error('rm-boom'); + return origRemove(node); + }; + + // Drop keys 2 and 3. Key 2 is processed cleanly; key 3 refuses part-way. + assert.throws(() => { render(rows([{ id: 1, n: 'one' }]), container); }, /rm-boom/); + ul.removeChild = origRemove; + + // Key 2 came out of the map before its row was touched, so re-adding it + // builds fresh. It used to stay mapped, and the detached instance was + // moved back in with its refs already unbound. + render(rows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }]), container); + const twos = [...container.querySelectorAll('li')].filter((li) => li.textContent === 'two'); + assert.equal(twos.length, 1, 'exactly one row for the re-added key'); + assert.notEqual(twos[0], liTwo, 'a disposed instance must not be resurrected'); + + // The survivor keeps its identity, and nothing is duplicated. + const ones = [...container.querySelectorAll('li')].filter((li) => li.textContent === 'one'); + assert.equal(ones.length, 1); + + // `liThree` is still in the document. That is the one residual this repair + // names and does not cover: `removeBetween` itself refused, and it only + // ever calls removeChild on nodes the renderer owns, so nothing short of + // the DOM lying can reach it. + assert.equal(liThree.parentNode, ul); +}); + +test('clearInstance: a throwing ref unbind does not wedge template swaps', () => { + const container = document.createElement('div'); + render(html`

A

`, container); + assert.equal(container.querySelector('p')?.textContent, 'A'); + + // A template-SHAPE swap runs the container-level teardown. The throw used + // to skip the replaceChildren() at the end of it, so the old DOM stayed and + // the instance was never replaced. `lastTarget` is cleared only after the + // throwing write, so it was permanent: every later swap threw at the same + // part, which is why this asserts TWO swaps. + render(html`B`, container); + assert.equal(container.querySelector('b')?.textContent, 'B'); + assert.equal(container.querySelector('p'), null); + + render(html`C`, container); + assert.equal(container.querySelector('i')?.textContent, 'C'); +}); + test('a plain template child hole recovers too (not just repeat)', () => { const container = document.createElement('div'); const view = (x) => html`
${x}
`; diff --git a/website/app/docs/error-handling/page.ts b/website/app/docs/error-handling/page.ts index 4f9dad74b..9c4aadb60 100644 --- a/website/app/docs/error-handling/page.ts +++ b/website/app/docs/error-handling/page.ts @@ -128,6 +128,7 @@ export default function GlobalError({ error }: { error: Error }) {

A directive that throws mid-commit stays consistent

The component boundary above also covers watch(signal) and until(), which commit outside the update cycle, so a throw from either reaches renderError() rather than the window. It reaches the component whose template holds the binding, which is not always the element the binding sits inside: a watch() written between a child component's tags belongs to the parent that wrote it. asyncAppend / asyncReplace are not covered, in two ways: that path logs its own iteration throw and continues on purpose, and a watch() or until() nested inside a chunk it commits still reaches the window.

Beyond reporting the error, the directive's own state is left describing the DOM that actually exists, which is what makes the NEXT render correct. That matters because the failure is otherwise silent: the renders that expose it are fully valid and log nothing. The hole whose commit threw is marked so the next render re-applies it instead of skipping it as unchanged, which is what used to leave a region blank for good. repeat() additionally repairs its key map so it describes the DOM again, and the next render is an ordinary reconcile that repositions every row (the symptom was a permanently duplicated row); it deliberately does not rebuild the region, which would throw away the node identity keyed reconciliation exists to preserve. guard() records its new deps only once the commit succeeds, so a later render with those deps re-renders the region instead of skipping past one the throw had blanked; until() advances its resolved priority only after its commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it.

+

Tearing content back out is covered too, and it has to be, because a teardown has no next render to repair it. Unbinding a ref while a row is removed can never abort the removal of the rest of the list, and repeat() drops each leftover key from its map before touching that row, so a row is either gone from both the map and the document or still in both, never removed but still mapped. Without that, a throw part-way through left the row you had DELETED on screen, reordered the survivors, and let a later render that re-added the key reinsert the disposed instance. The cost is that a ref whose callback or whose object value setter throws is swallowed on teardown (lit propagates from both, so this is a deliberate divergence). It applies to teardown only: on the commit path a throwing ref still reaches renderError().

Server action errors

Errors thrown from server actions are sanitized in production: the client gets a generic "Internal server error" message plus a short digest, never the raw thrown message or the stack trace. The full error is logged server-side keyed by that digest, so a client-reported digest maps back to the server log line. A redirect() / notFound() control-flow throw passes through. To surface a specific user-facing message, return an ActionResult { success: false, error } envelope instead of throwing.

From 320a28a43460f55a2e0443302d51adccfef59d7b Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 15:57:01 +0530 Subject: [PATCH 2/6] fix: remove and unmap a repeat() leftover together, not one before the other Unmapping the key before touching the row held the invariant for a dispose throw and broke its mirror image: a removal that itself refused left the row in the document described by neither map, which is the untracked-orphan class this repair exists to close. Removing and unmapping in one finally holds both ends, since a row that could not leave the document keeps its key. Also corrects the comment and both doc surfaces on which ref branch does what. applyElement guards every callback-ref call on the commit path too, so a throwing ref CALLBACK was already swallowed everywhere; the object ref was guarded on neither. This makes the two agree on teardown and leaves the commit path alone, so the claim that a commit throw still reaches the boundary is now true only where it is, the object-ref writes. The residual is scoped rather than asserted away: a refused DOM removal costs that row its position, and cannot be cleaned up later, since removeBetween returns early once the start marker is gone. Named in the comment and asserted in the browser test rather than glossed. Adds the browser assertion the loop shape had no coverage for, driving the throw through removeChild rather than the ref unbind the guard makes unable to throw. --- .agents/skills/webjs/references/components.md | 2 +- packages/core/src/render-client.js | 74 ++++++++++++------- .../browser/directive-commit-throw.test.js | 45 +++++++++++ .../rendering/directive-commit-throw.test.js | 35 +++++---- website/app/docs/error-handling/page.ts | 2 +- 5 files changed, 111 insertions(+), 47 deletions(-) diff --git a/.agents/skills/webjs/references/components.md b/.agents/skills/webjs/references/components.md index 77c53664c..6a0780e6d 100644 --- a/.agents/skills/webjs/references/components.md +++ b/.agents/skills/webjs/references/components.md @@ -183,7 +183,7 @@ The boundary also covers `watch(signal)` (its notify microtask) and `until()` (i **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. -**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 a row is either gone from both the map and the document or still in both, never removed but still mapped (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 callback or whose object `value` setter throws is SWALLOWED on teardown. That is a deliberate divergence from lit, which propagates from both, and it applies only to teardown: on the commit path a throwing `ref` still reaches `renderError()`, because there the boundary can report it and the next render can repair 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()` removes each leftover row and drops its key together, so a row is either gone from both the map and the document or still in both, never one without the other (it 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 `` 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. diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index e52d889bc..363cd5bee 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -1773,12 +1773,17 @@ function disposeInstance(inst) { // 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. An object ref whose setter throws and a callback - // ref that throws are the same act by the author, and they get the - // same contract. This is a deliberate divergence from lit, recorded in - // the docs. The COMMIT path is different and stays unguarded (see - // `applyElement`): a commit throw has a route (the component boundary) - // and a repair, so swallowing there would hide a real author error. + // 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') { @@ -1861,18 +1866,23 @@ function reconcileRepeat(part, value) { } } - // 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 still in the - // document, and a row whose dispose threw still leaves it. Iterating a - // snapshot keeps the delete obviously safe rather than relying on the - // reader knowing that deleting during a Map iteration is legal. + // Remove any keys that remain in the old map. Both the removal and the + // unmapping sit in a `finally`, in that order, so at any throw point + // `state.map` holds exactly the rows still in the document: a dispose + // throw still removes the row AND drops its key, while a removal that + // itself refuses keeps the key, because the row it failed to remove is + // still there. Unmapping FIRST would trade one broken invariant for its + // mirror image, leaving that row in the document described by neither + // map, which is the untracked-orphan class this whole repair is about. + // 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.delete(k); } } state.map = newMap; @@ -1894,19 +1904,29 @@ function reconcileRepeat(part, value) { // 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 deletes each key before - // touching its row and removes the nodes in a `finally`, so a leftover is - // either fully gone from both the map and the document or still in both, - // never removed-but-still-mapped. Without that, a throw mid-removal would - // merge `newMap` over a `state.map` still holding disposed, detached - // rows: the row the app DELETED stays on screen, the survivors reorder, - // and a later render that re-adds that key reinserts the detached - // instance. The invariant, at any throw point on either branch: every - // instance still in the document 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. - // One residual: a throw from `removeBetween` itself, which only calls - // `removeChild` on nodes the renderer owns. + // because the loop was written to earn it. Each leftover is removed and + // unmapped together in a `finally`, so it is either gone from both the + // map and the document or still in both, never one without the other. + // Without that, a throw mid-removal would merge `newMap` over a + // `state.map` still holding disposed, detached rows: the row the app + // DELETED stays on screen, the survivors reorder, and a later render that + // re-adds that key reinserts the detached instance. The invariant, at any + // throw point on either branch: every instance still in the document is + // described by exactly one of the two maps, `newMap` for the processed + // new keys and `state.map` for the leftovers not fully removed, which is + // what makes the merge below correct. + // + // The residual, scoped honestly: if `removeBetween` itself refuses part + // way (it only calls `removeChild` on nodes the renderer owns, so it + // takes a throwing DOM), that row keeps its key and its remaining nodes, + // so the invariant above still holds AT THE THROW and a re-add reuses the + // row rather than duplicating it. What that row loses is its POSITION, + // since the removal already took its start marker and `moveRange` has no + // range left to move. And the guarantee is not permanent: those nodes can + // never be removed afterwards either, because `removeBetween` returns + // early once the start marker is gone, so the next pass that treats the + // key as a leftover unmaps it and the remnant is left untracked. One + // refusing DOM removal costs one row, deferred rather than prevented. // // Deliberately NOT a teardown-and-rebuild of the region. Rebuilding is // the obvious defensive move and it is measurably worse: it discards node @@ -1935,11 +1955,11 @@ function teardownRepeat(state) { // 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.delete(k); } } state.map.clear(); diff --git a/packages/core/test/rendering/browser/directive-commit-throw.test.js b/packages/core/test/rendering/browser/directive-commit-throw.test.js index 956f8d459..124b4e4f3 100644 --- a/packages/core/test/rendering/browser/directive-commit-throw.test.js +++ b/packages/core/test/rendering/browser/directive-commit-throw.test.js @@ -240,6 +240,51 @@ suite('directive commit throws (browser)', () => { assert.ok(after[1] !== before[1], 'the disposed instance must not be resurrected'); }); + test('a refused DOM removal keeps that row keyed, and does not duplicate it', () => { + // The ref-unbind case above cannot reach the removal loop's own shape, + // because the guard makes that step unable to throw at all. This drives + // the throw from the DOM removal instead, in a real browser, where node + // identity is the thing that separates "reused the row already there" + // from "built a second one beside it". + const idRows = (items) => html`
    ${repeat( + items, + (it) => it.id, + (it) => html`
  • ${it.n}
    • `, + )}
`; + + render(idRows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 3, n: 'three' }]), container); + const [liOne, liTwo, liThree] = [...container.querySelectorAll('li')]; + + const ul = container.querySelector('ul'); + const origRemove = ul.removeChild.bind(ul); + ul.removeChild = (node) => { + if (node === liThree) throw new Error('rm-boom'); + return origRemove(node); + }; + throwsMatching(() => { render(idRows([{ id: 1, n: 'one' }]), container); }, /rm-boom/); + ul.removeChild = origRemove; + + render(idRows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 3, n: 'three' }]), container); + const after = [...container.querySelectorAll('li')]; + + // Every key renders exactly once. Key 1 never left, so it is the same + // element. Key 2 left the map together with its row, so it MISSES and + // rebuilds. Key 3 never left either, because its row never left the + // document, so it HITS and reuses the row already there instead of + // building a second one beside it, which is the whole point. + assert.equal(after.length, 3); + assert.strictEqual(after.filter((li) => li === liOne).length, 1); + assert.strictEqual(after.filter((li) => li === liThree).length, 1); + assert.ok(!after.includes(liTwo), 'a removed row must not be resurrected'); + assert.deepEqual([...after].map((li) => li.textContent).sort(), ['one', 'three', 'two']); + + // Its ORDER is the residual, and it is asserted rather than glossed: the + // refused removal already took the row's start marker, so the reconciler + // can no longer move that range and the row keeps whatever slot it had. + // One refusing DOM removal costs that row its position. + assert.strictEqual(after[0], liThree); + }); + test('removing rows after recovery leaves nothing behind', () => { render(rows(good), container); throwsMatching(() => { diff --git a/packages/core/test/rendering/directive-commit-throw.test.js b/packages/core/test/rendering/directive-commit-throw.test.js index 56b4f6278..cb762942b 100644 --- a/packages/core/test/rendering/directive-commit-throw.test.js +++ b/packages/core/test/rendering/directive-commit-throw.test.js @@ -249,27 +249,26 @@ test('repeat: a throw INSIDE the removal loop leaves no leftover still mapped', return origRemove(node); }; - // Drop keys 2 and 3. Key 2 is processed cleanly; key 3 refuses part-way. + // Drop keys 2 and 3. Key 2 is removed and unmapped together; key 3's DOM + // removal refuses part-way, so it keeps BOTH its nodes and its key. assert.throws(() => { render(rows([{ id: 1, n: 'one' }]), container); }, /rm-boom/); ul.removeChild = origRemove; + assert.equal(liThree.parentNode, ul, 'the refused removal leaves its nodes behind'); + + // One render that re-adds both, which is where the two halves of the + // invariant show up as opposite outcomes. Key 2 left the map with its row, + // so it MISSES and builds fresh (it used to stay mapped, and the detached + // instance was moved back in with its refs already unbound). Key 3 never + // left the map, because its row never left the document, so it HITS and + // reuses the row already there rather than building a duplicate beside it. + render(rows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 3, n: 'three' }]), container); + const at = (text) => [...container.querySelectorAll('li')].filter((li) => li.textContent === text); - // Key 2 came out of the map before its row was touched, so re-adding it - // builds fresh. It used to stay mapped, and the detached instance was - // moved back in with its refs already unbound. - render(rows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }]), container); - const twos = [...container.querySelectorAll('li')].filter((li) => li.textContent === 'two'); - assert.equal(twos.length, 1, 'exactly one row for the re-added key'); - assert.notEqual(twos[0], liTwo, 'a disposed instance must not be resurrected'); - - // The survivor keeps its identity, and nothing is duplicated. - const ones = [...container.querySelectorAll('li')].filter((li) => li.textContent === 'one'); - assert.equal(ones.length, 1); - - // `liThree` is still in the document. That is the one residual this repair - // names and does not cover: `removeBetween` itself refused, and it only - // ever calls removeChild on nodes the renderer owns, so nothing short of - // the DOM lying can reach it. - assert.equal(liThree.parentNode, ul); + assert.equal(at('one').length, 1); + assert.equal(at('two').length, 1, 'exactly one row for the re-added key'); + assert.notEqual(at('two')[0], liTwo, 'a disposed instance must not be resurrected'); + assert.equal(at('three').length, 1, 'the row that could not be removed must not be duplicated'); + assert.equal(at('three')[0], liThree); }); test('clearInstance: a throwing ref unbind does not wedge template swaps', () => { diff --git a/website/app/docs/error-handling/page.ts b/website/app/docs/error-handling/page.ts index 9c4aadb60..de0e26023 100644 --- a/website/app/docs/error-handling/page.ts +++ b/website/app/docs/error-handling/page.ts @@ -128,7 +128,7 @@ export default function GlobalError({ error }: { error: Error }) {

A directive that throws mid-commit stays consistent

The component boundary above also covers watch(signal) and until(), which commit outside the update cycle, so a throw from either reaches renderError() rather than the window. It reaches the component whose template holds the binding, which is not always the element the binding sits inside: a watch() written between a child component's tags belongs to the parent that wrote it. asyncAppend / asyncReplace are not covered, in two ways: that path logs its own iteration throw and continues on purpose, and a watch() or until() nested inside a chunk it commits still reaches the window.

Beyond reporting the error, the directive's own state is left describing the DOM that actually exists, which is what makes the NEXT render correct. That matters because the failure is otherwise silent: the renders that expose it are fully valid and log nothing. The hole whose commit threw is marked so the next render re-applies it instead of skipping it as unchanged, which is what used to leave a region blank for good. repeat() additionally repairs its key map so it describes the DOM again, and the next render is an ordinary reconcile that repositions every row (the symptom was a permanently duplicated row); it deliberately does not rebuild the region, which would throw away the node identity keyed reconciliation exists to preserve. guard() records its new deps only once the commit succeeds, so a later render with those deps re-renders the region instead of skipping past one the throw had blanked; until() advances its resolved priority only after its commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it.

-

Tearing content back out is covered too, and it has to be, because a teardown has no next render to repair it. Unbinding a ref while a row is removed can never abort the removal of the rest of the list, and repeat() drops each leftover key from its map before touching that row, so a row is either gone from both the map and the document or still in both, never removed but still mapped. Without that, a throw part-way through left the row you had DELETED on screen, reordered the survivors, and let a later render that re-added the key reinsert the disposed instance. The cost is that a ref whose callback or whose object value setter throws is swallowed on teardown (lit propagates from both, so this is a deliberate divergence). It applies to teardown only: on the commit path a throwing ref still reaches renderError().

+

Tearing content back out is covered too, and it has to be, because a teardown has no next render to repair it. Unbinding a ref while a row is removed can never abort the removal of the rest of the list, and repeat() removes each leftover row and drops its key together, so a row is either gone from both the map and the document or still in both, never one without the other. Without that, a throw part-way through left the row you had DELETED on screen, reordered the survivors, and let a later render that re-added the key reinsert the disposed instance. The cost is that a ref whose object value setter throws is swallowed on teardown, matching the ref callback, which was already swallowed everywhere (lit guards neither and propagates from both, so this is a deliberate divergence). It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches renderError().

Server action errors

Errors thrown from server actions are sanitized in production: the client gets a generic "Internal server error" message plus a short digest, never the raw thrown message or the stack trace. The full error is logged server-side keyed by that digest, so a client-reported digest maps back to the server log line. A redirect() / notFound() control-flow throw passes through. To surface a specific user-facing message, return an ActionResult { success: false, error } envelope instead of throwing.

From b5d0677f6438eb9b35ccb5a295b053388f61c1f4 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 16:10:53 +0530 Subject: [PATCH 3/6] fix: keep unmapping a repeat() leftover before its row, not after Reverts the ordering flip from the previous commit, which was worse than what it replaced, and says why in the comment so it does not get flipped again. Unmapping after the removal keeps the key when removeBetween itself refuses. That row is then half removed, its start marker gone and its end marker still in place, so a re-add hits the reuse branch and moveRange re-attaches the lone start marker AFTER its own end marker. The next removal of that key walks forward from a start that never reaches its end, so it takes the repeat part's marker and every following sibling with it, and the region is dead for good. Measured on the same fixture the tests use. Unmapping first, then dropping key 3 and adding key 4: three, one, two then three, one, two, four, so the list keeps reconciling around one untracked remnant. Unmapping last: three then three, with no later render ever landing again. So the residual is one untracked row after a refusing DOM removal, and it is the right trade against a destroyed list. Both the comment and the tests now state that rather than the reverse. The ref-contract correction from the previous commit stands: applyElement guards every callback-ref call on the commit path, so only the object writes propagate there. --- .agents/skills/webjs/references/components.md | 2 +- packages/core/src/render-client.js | 58 +++++++++---------- .../browser/directive-commit-throw.test.js | 31 +++++----- .../rendering/directive-commit-throw.test.js | 35 ++++++----- website/app/docs/error-handling/page.ts | 2 +- 5 files changed, 67 insertions(+), 61 deletions(-) diff --git a/.agents/skills/webjs/references/components.md b/.agents/skills/webjs/references/components.md index 6a0780e6d..7df1f4453 100644 --- a/.agents/skills/webjs/references/components.md +++ b/.agents/skills/webjs/references/components.md @@ -183,7 +183,7 @@ The boundary also covers `watch(signal)` (its notify microtask) and `until()` (i **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. -**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()` removes each leftover row and drops its key together, so a row is either gone from both the map and the document or still in both, never one without the other (it 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. +**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 `` 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. diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index 363cd5bee..b64680b1b 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -1866,23 +1866,19 @@ function reconcileRepeat(part, value) { } } - // Remove any keys that remain in the old map. Both the removal and the - // unmapping sit in a `finally`, in that order, so at any throw point - // `state.map` holds exactly the rows still in the document: a dispose - // throw still removes the row AND drops its key, while a removal that - // itself refuses keeps the key, because the row it failed to remove is - // still there. Unmapping FIRST would trade one broken invariant for its - // mirror image, leaving that row in the document described by neither - // map, which is the untracked-orphan class this whole repair is about. + // 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.delete(k); } } state.map = newMap; @@ -1904,29 +1900,29 @@ function reconcileRepeat(part, value) { // 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. Each leftover is removed and - // unmapped together in a `finally`, so it is either gone from both the - // map and the document or still in both, never one without the other. - // Without that, a throw mid-removal would merge `newMap` over a - // `state.map` still holding disposed, detached rows: the row the app - // DELETED stays on screen, the survivors reorder, and a later render that - // re-adds that key reinserts the detached instance. The invariant, at any - // throw point on either branch: every instance still in the document is - // described by exactly one of the two maps, `newMap` for the processed - // new keys and `state.map` for the leftovers not fully removed, which is + // 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 still in the document + // 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 residual, scoped honestly: if `removeBetween` itself refuses part - // way (it only calls `removeChild` on nodes the renderer owns, so it - // takes a throwing DOM), that row keeps its key and its remaining nodes, - // so the invariant above still holds AT THE THROW and a re-add reuses the - // row rather than duplicating it. What that row loses is its POSITION, - // since the removal already took its start marker and `moveRange` has no - // range left to move. And the guarantee is not permanent: those nodes can - // never be removed afterwards either, because `removeBetween` returns - // early once the start marker is gone, so the next pass that treats the - // key as a leftover unmaps it and the remnant is left untracked. One - // refusing DOM removal costs one row, deferred rather than prevented. + // 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 @@ -1955,11 +1951,11 @@ function teardownRepeat(state) { // 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.delete(k); } } state.map.clear(); diff --git a/packages/core/test/rendering/browser/directive-commit-throw.test.js b/packages/core/test/rendering/browser/directive-commit-throw.test.js index 124b4e4f3..6f553a3d9 100644 --- a/packages/core/test/rendering/browser/directive-commit-throw.test.js +++ b/packages/core/test/rendering/browser/directive-commit-throw.test.js @@ -264,25 +264,26 @@ suite('directive commit throws (browser)', () => { throwsMatching(() => { render(idRows([{ id: 1, n: 'one' }]), container); }, /rm-boom/); ul.removeChild = origRemove; - render(idRows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 3, n: 'three' }]), container); + render(idRows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }]), container); const after = [...container.querySelectorAll('li')]; - // Every key renders exactly once. Key 1 never left, so it is the same - // element. Key 2 left the map together with its row, so it MISSES and - // rebuilds. Key 3 never left either, because its row never left the - // document, so it HITS and reuses the row already there instead of - // building a second one beside it, which is the whole point. - assert.equal(after.length, 3); + // Key 1 never left the map, so it is the same element. Key 2 left the map + // together with its row, so it MISSES and rebuilds rather than having a + // disposed instance handed back. assert.strictEqual(after.filter((li) => li === liOne).length, 1); - assert.strictEqual(after.filter((li) => li === liThree).length, 1); assert.ok(!after.includes(liTwo), 'a removed row must not be resurrected'); - assert.deepEqual([...after].map((li) => li.textContent).sort(), ['one', 'three', 'two']); - - // Its ORDER is the residual, and it is asserted rather than glossed: the - // refused removal already took the row's start marker, so the reconciler - // can no longer move that range and the row keeps whatever slot it had. - // One refusing DOM removal costs that row its position. - assert.strictEqual(after[0], liThree); + assert.strictEqual(after.filter((li) => li.textContent === 'two').length, 1); + + // `liThree` is the named residual: the DOM removal itself refused, so + // those nodes stayed, and its key was already dropped, so nothing tracks + // them. What the trade buys is that the region still RECONCILES, which is + // the assertion that matters and the one only a real browser settles. + assert.strictEqual(liThree.parentNode, ul); + render(idRows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 4, n: 'four' }]), container); + assert.deepEqual( + [...container.querySelectorAll('li')].map((li) => li.textContent).filter((t) => t !== 'three'), + ['one', 'two', 'four'], + ); }); test('removing rows after recovery leaves nothing behind', () => { diff --git a/packages/core/test/rendering/directive-commit-throw.test.js b/packages/core/test/rendering/directive-commit-throw.test.js index cb762942b..ff7d20a5d 100644 --- a/packages/core/test/rendering/directive-commit-throw.test.js +++ b/packages/core/test/rendering/directive-commit-throw.test.js @@ -249,26 +249,35 @@ test('repeat: a throw INSIDE the removal loop leaves no leftover still mapped', return origRemove(node); }; - // Drop keys 2 and 3. Key 2 is removed and unmapped together; key 3's DOM - // removal refuses part-way, so it keeps BOTH its nodes and its key. + // Drop keys 2 and 3. Key 2 is processed cleanly; key 3's DOM removal + // refuses part-way. assert.throws(() => { render(rows([{ id: 1, n: 'one' }]), container); }, /rm-boom/); ul.removeChild = origRemove; - assert.equal(liThree.parentNode, ul, 'the refused removal leaves its nodes behind'); - - // One render that re-adds both, which is where the two halves of the - // invariant show up as opposite outcomes. Key 2 left the map with its row, - // so it MISSES and builds fresh (it used to stay mapped, and the detached - // instance was moved back in with its refs already unbound). Key 3 never - // left the map, because its row never left the document, so it HITS and - // reuses the row already there rather than building a duplicate beside it. - render(rows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 3, n: 'three' }]), container); + + // Key 2 came out of the map before its row was touched, so re-adding it + // builds fresh. It used to stay mapped, and the detached instance was + // moved back in with its refs already unbound. + render(rows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }]), container); const at = (text) => [...container.querySelectorAll('li')].filter((li) => li.textContent === text); assert.equal(at('one').length, 1); assert.equal(at('two').length, 1, 'exactly one row for the re-added key'); assert.notEqual(at('two')[0], liTwo, 'a disposed instance must not be resurrected'); - assert.equal(at('three').length, 1, 'the row that could not be removed must not be duplicated'); - assert.equal(at('three')[0], liThree); + + // `liThree` is the named residual: `removeBetween` itself refused, so those + // nodes stayed, and the key was already dropped, so nothing tracks them. + // The list still RECONCILES, which is what the residual is traded for. Do + // not "fix" this by unmapping after the removal instead: that keeps the key + // pointing at a half-removed row whose start marker is gone, the re-add + // re-attaches that marker after its own end marker, and the next removal + // walks off the end and takes the whole region with it. + assert.equal(liThree.parentNode, ul); + render(rows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 4, n: 'four' }]), container); + assert.deepEqual( + [...container.querySelectorAll('li')].map((li) => li.textContent).filter((t) => t !== 'three'), + ['one', 'two', 'four'], + 'the region keeps reconciling around the untracked remnant', + ); }); test('clearInstance: a throwing ref unbind does not wedge template swaps', () => { diff --git a/website/app/docs/error-handling/page.ts b/website/app/docs/error-handling/page.ts index de0e26023..d678389a9 100644 --- a/website/app/docs/error-handling/page.ts +++ b/website/app/docs/error-handling/page.ts @@ -128,7 +128,7 @@ export default function GlobalError({ error }: { error: Error }) {

A directive that throws mid-commit stays consistent

The component boundary above also covers watch(signal) and until(), which commit outside the update cycle, so a throw from either reaches renderError() rather than the window. It reaches the component whose template holds the binding, which is not always the element the binding sits inside: a watch() written between a child component's tags belongs to the parent that wrote it. asyncAppend / asyncReplace are not covered, in two ways: that path logs its own iteration throw and continues on purpose, and a watch() or until() nested inside a chunk it commits still reaches the window.

Beyond reporting the error, the directive's own state is left describing the DOM that actually exists, which is what makes the NEXT render correct. That matters because the failure is otherwise silent: the renders that expose it are fully valid and log nothing. The hole whose commit threw is marked so the next render re-applies it instead of skipping it as unchanged, which is what used to leave a region blank for good. repeat() additionally repairs its key map so it describes the DOM again, and the next render is an ordinary reconcile that repositions every row (the symptom was a permanently duplicated row); it deliberately does not rebuild the region, which would throw away the node identity keyed reconciliation exists to preserve. guard() records its new deps only once the commit succeeds, so a later render with those deps re-renders the region instead of skipping past one the throw had blanked; until() advances its resolved priority only after its commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it.

-

Tearing content back out is covered too, and it has to be, because a teardown has no next render to repair it. Unbinding a ref while a row is removed can never abort the removal of the rest of the list, and repeat() removes each leftover row and drops its key together, so a row is either gone from both the map and the document or still in both, never one without the other. Without that, a throw part-way through left the row you had DELETED on screen, reordered the survivors, and let a later render that re-added the key reinsert the disposed instance. The cost is that a ref whose object value setter throws is swallowed on teardown, matching the ref callback, which was already swallowed everywhere (lit guards neither and propagates from both, so this is a deliberate divergence). It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches renderError().

+

Tearing content back out is covered too, and it has to be, because a teardown has no next render to repair it. Unbinding a ref while a row is removed can never abort the removal of the rest of the list, 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. Without that, a throw part-way through left the row you had DELETED on screen, reordered the survivors, and let a later render that re-added the key reinsert the disposed instance. The cost is that a ref whose object value setter throws is swallowed on teardown, matching the ref callback, which was already swallowed everywhere (lit guards neither and propagates from both, so this is a deliberate divergence). It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches renderError().

Server action errors

Errors thrown from server actions are sanitized in production: the client gets a generic "Internal server error" message plus a short digest, never the raw thrown message or the stack trace. The full error is logged server-side keyed by that digest, so a client-reported digest maps back to the server log line. A redirect() / notFound() control-flow throw passes through. To surface a specific user-facing message, return an ActionResult { success: false, error } envelope instead of throwing.

From 22a06d3cce945fa9b18b1360a773239874aae014 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 16:30:20 +0530 Subject: [PATCH 4/6] fix: pin the leftover-unmap ordering with a test, and use it in the walk too The ordering the previous commit restored had nothing holding it: flipping the loops back left every test green, because that commit had also shortened the sequence past the render where the two orderings differ. The test now re-adds the REFUSED key before the next removal, which is the only point they diverge, and asserts both the fresh row beside the remnant and that a later render still lands. The walk's own shape-mismatch branch was still unmapping after the removal, which is the ordering the leftover loop's comment calls fatal, and it produces the destroyed region verbatim for a row whose shape changes and changes back. Both branches now unmap first, so the invariant the catch relies on holds across the whole function rather than half of it. The invariant sentence said every instance in the document is described by exactly one of the two maps, which the residual three lines down contradicts. It now excludes the row whose removal refused, which is in neither, by choice. --- packages/core/src/render-client.js | 18 +++++++--- .../rendering/directive-commit-throw.test.js | 33 +++++++++++-------- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index b64680b1b..7b54427ac 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -1856,9 +1856,15 @@ 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); @@ -1906,10 +1912,12 @@ function reconcileRepeat(part, value) { // 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 still in the document - // 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. + // 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 diff --git a/packages/core/test/rendering/directive-commit-throw.test.js b/packages/core/test/rendering/directive-commit-throw.test.js index ff7d20a5d..409c34df9 100644 --- a/packages/core/test/rendering/directive-commit-throw.test.js +++ b/packages/core/test/rendering/directive-commit-throw.test.js @@ -254,29 +254,36 @@ test('repeat: a throw INSIDE the removal loop leaves no leftover still mapped', assert.throws(() => { render(rows([{ id: 1, n: 'one' }]), container); }, /rm-boom/); ul.removeChild = origRemove; - // Key 2 came out of the map before its row was touched, so re-adding it - // builds fresh. It used to stay mapped, and the detached instance was - // moved back in with its refs already unbound. - render(rows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }]), container); + // `liThree` is the named residual: `removeBetween` itself refused, so those + // nodes stayed, and the key was already dropped, so nothing tracks them. + assert.equal(liThree.parentNode, ul); + + // Re-add BOTH dropped keys in ONE render, with no render in between. That + // ordering is load-bearing rather than incidental: any render that treats + // key 3 as a leftover again unmaps it under EITHER unmap ordering (its + // start marker is gone, so `removeBetween` early-returns and the delete + // runs), which collapses the difference this test exists to catch. Re-added + // immediately, key 3 is already unmapped, so it MISSES and builds a fresh + // row beside the remnant. Unmapping after the removal instead would keep + // that key pointing at a half-removed row, the re-add would take the reuse + // branch, and `moveRange` would re-attach the lone start marker AFTER its + // own end marker. + render(rows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 3, n: 'three' }]), container); const at = (text) => [...container.querySelectorAll('li')].filter((li) => li.textContent === text); assert.equal(at('one').length, 1); assert.equal(at('two').length, 1, 'exactly one row for the re-added key'); assert.notEqual(at('two')[0], liTwo, 'a disposed instance must not be resurrected'); + assert.equal(at('three').length, 2, 'a fresh row for the re-added key, beside the remnant'); - // `liThree` is the named residual: `removeBetween` itself refused, so those - // nodes stayed, and the key was already dropped, so nothing tracks them. - // The list still RECONCILES, which is what the residual is traded for. Do - // not "fix" this by unmapping after the removal instead: that keeps the key - // pointing at a half-removed row whose start marker is gone, the re-add - // re-attaches that marker after its own end marker, and the next removal - // walks off the end and takes the whole region with it. - assert.equal(liThree.parentNode, ul); + // And the region is still alive. Under the other ordering the removal below + // walks off the end of the mis-ordered range and takes the repeat part's + // own marker with it, after which no render ever lands again. render(rows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 4, n: 'four' }]), container); assert.deepEqual( [...container.querySelectorAll('li')].map((li) => li.textContent).filter((t) => t !== 'three'), ['one', 'two', 'four'], - 'the region keeps reconciling around the untracked remnant', + 'the region still reconciles rather than being dead', ); }); From e540bae6e5dfdc5660e2f99587fcc51ad54e3f1e Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 15:46:06 +0530 Subject: [PATCH 5/6] fix: a mid-commit throw no longer strands a row in a plain .map() array reconcileArray accumulated its replacement slot list locally and committed it only after the whole walk, so a throw part-way discarded the list entirely. The tracked slots kept describing positions whose nodes were already removed, while the freshly built ones sat in the document tracked by nothing. The orphan then outlived every later render including an empty one, because the only code that could remove it walks the tracked list. Only the shape-changed branch is destructive (it inserts the replacement and removes the old slot before the loop can finish), which is why #1172 read the common same-shape path as leaving the DOM untouched. The repair splices the untouched tail of the old list onto what the pass accumulated, the array analogue of reconcileRepeat's catch, and rethrows. The boundary comes from a processed-slot cursor rather than the new list's length because the shrink loop advances through the old slots while the new list stops growing; splicing from the length would re-describe an already-removed slot, and a later render that grew the array would match a live value against a detached slot and that row would silently never appear. The two destructive branches also push before they remove, a pure reordering on the success path that keeps a built and inserted slot tracked at every throw point. It is not a substitute for the catch: it makes the failed POSITION atomic, while the corruption is that the whole list was committed late. --- .agents/skills/webjs/references/components.md | 2 +- packages/core/src/render-client.js | 113 +++++++++++++----- .../browser/directive-commit-throw.test.js | 31 +++++ .../rendering/directive-commit-throw.test.js | 85 +++++++++++++ website/app/docs/error-handling/page.ts | 2 +- 5 files changed, 201 insertions(+), 32 deletions(-) diff --git a/.agents/skills/webjs/references/components.md b/.agents/skills/webjs/references/components.md index 7df1f4453..02038574f 100644 --- a/.agents/skills/webjs/references/components.md +++ b/.agents/skills/webjs/references/components.md @@ -181,7 +181,7 @@ 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`${watch(sig)}`` 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 when an item's TEMPLATE SHAPE changes in the render that throws, 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. diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index 7b54427ac..56a3a0d24 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -2055,42 +2055,95 @@ 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. The + // push comes FIRST for the same reason as in the branch below. + next.push({ type: 'empty' }); + if (o) removeArrayItem(o); + 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. + // + // Two residuals, both from the removal step itself rather than the + // bookkeeping. A throw out of `removeArrayItem` leaves the slot it was + // removing described one position later than it sits, costing that row + // its identity on the next render but orphaning nothing (and it takes a + // throwing DOM to reach at all, since the teardown it calls is total). + // Beyond that only `removeBetween` can throw, and it calls `removeChild` + // solely on nodes the renderer owns. + state.items = next.concat(old.slice(consumed)); + throw err; + } } /** diff --git a/packages/core/test/rendering/browser/directive-commit-throw.test.js b/packages/core/test/rendering/browser/directive-commit-throw.test.js index 6f553a3d9..ea27437f3 100644 --- a/packages/core/test/rendering/browser/directive-commit-throw.test.js +++ b/packages/core/test/rendering/browser/directive-commit-throw.test.js @@ -286,6 +286,37 @@ suite('directive commit throws (browser)', () => { ); }); + test('a plain .map() array recovers a shape-changed row without losing identity', () => { + // The non-keyed reconciler updates in place, so the rows that were NOT + // rebuilt must survive the recovery as the same elements. linkedom can + // show the markup is right; only a real DOM can show it was repaired + // rather than rebuilt. + const view = (items) => html`
${items.map((it) => ( + it.kind === 'a' ? html`

${it.v}

` : html`${it.v}` + ))}
`; + + render(view([{ kind: 'a', v: '1' }, { kind: 'a', v: '2' }]), container); + const before = [...container.querySelectorAll('p')]; + + throwsMatching(() => { + render(view([{ kind: 'b', v: '1' }, { kind: 'a', v: poison }]), container); + }, /boom/); + + render(view([{ kind: 'b', v: '1' }, { kind: 'a', v: '2' }]), container); + const region = container.querySelector('div'); + assert.deepEqual( + [...region.children].map((el) => `${el.tagName.toLowerCase()}:${el.textContent}`), + ['b:1', 'p:2'], + ); + // Row 1 changed shape and was legitimately rebuilt; row 2 did not, and + // holding its identity is what proves this is a reconcile against + // repaired bookkeeping rather than a teardown of the region. + assert.strictEqual(region.querySelector('p'), before[1]); + + render(view([]), container); + assert.equal(container.querySelector('div').children.length, 0); + }); + test('removing rows after recovery leaves nothing behind', () => { render(rows(good), container); throwsMatching(() => { diff --git a/packages/core/test/rendering/directive-commit-throw.test.js b/packages/core/test/rendering/directive-commit-throw.test.js index 409c34df9..e2bac6b3d 100644 --- a/packages/core/test/rendering/directive-commit-throw.test.js +++ b/packages/core/test/rendering/directive-commit-throw.test.js @@ -316,6 +316,91 @@ test('a plain template child hole recovers too (not just repeat)', () => { assert.equal(container.querySelector('span').textContent, 'ok'); }); +// --- plain .map() arrays (the non-keyed child reconciler) --- + +/** Rendered markup of the array region, with the renderer's markers stripped. */ +function regionHTML(container) { + return container.querySelector('div').innerHTML.replace(//g, ''); +} + +// The shape-changed branch is the destructive one: it inserts the replacement +// and removes the old slot BEFORE the walk can finish. A same-shape update +// touches only values, so it cannot reach this at all, which is why every +// case below changes an item's template SHAPE in the render that throws. + +test('array: a mid-walk throw does not strand a row on the next valid render', () => { + const container = document.createElement('div'); + const view = (items) => html`
${items.map((it) => ( + it.kind === 'a' ? html`

${it.v}

` : html`${it.v}` + ))}
`; + + render(view([{ kind: 'a', v: '1' }, { kind: 'a', v: '2' }]), container); + assert.equal(regionHTML(container), '

1

2

'); + + // Item 0 changes shape (destructive) and item 1's child hole then throws. + assert.throws(() => { + render(view([{ kind: 'b', v: '1' }, { kind: 'a', v: poison }]), container); + }, /boom/); + + // The freshly built used to be tracked by nothing, so it survived + // alongside a rebuilt copy of itself: 11

2

. + render(view([{ kind: 'b', v: '1' }, { kind: 'a', v: '2' }]), container); + assert.equal(regionHTML(container), '1

2

'); + + // Not merely delayed by one render. + render(view([{ kind: 'b', v: '1' }, { kind: 'a', v: '2' }]), container); + assert.equal(regionHTML(container), '1

2

'); +}); + +test('array: an EMPTY render after a throw leaves nothing behind', () => { + const container = document.createElement('div'); + const view = (items) => html`
${items.map((it) => ( + it.kind === 'a' ? html`

${it.v}

` : html`${it.v}` + ))}
`; + + render(view([{ kind: 'a', v: '1' }, { kind: 'a', v: '2' }]), container); + assert.throws(() => { + render(view([{ kind: 'b', v: '1' }, { kind: 'a', v: poison }]), container); + }, /boom/); + + // The sharpest probe there is: the only code that could remove a slot walks + // the tracked list, so anything tracked by nothing outlives even a render + // that asks for no rows at all. + render(view([]), container); + assert.equal(regionHTML(container), ''); +}); + +test('array: a throw in the SHRINK loop leaves no slot describing a detached row', () => { + // The shrink loop advances through the old slots while the replacement list + // stops growing, so this is the case that separates the processed-slot + // cursor from the replacement list's length. Splicing from the latter would + // re-describe an already-removed slot, and the bug only surfaces later, on a + // render that GROWS the array back. + const container = document.createElement('div'); + const view = (items) => html`
${items.map((v) => html`

${v}

`)}
`; + + render(view(['1', '2', '3', '4']), container); + const region = container.querySelector('div'); + const fourth = [...region.querySelectorAll('p')][3]; + + const origRemove = region.removeChild.bind(region); + region.removeChild = (node) => { + if (node === fourth) throw new Error('rm-boom'); + return origRemove(node); + }; + + // Drop the last two. Slot 2 is removed cleanly, slot 3 refuses part-way. + assert.throws(() => { render(view(['1', '2']), container); }, /rm-boom/); + region.removeChild = origRemove; + + // Grow back. The already-removed slot must not still be described, or its + // detached instance matches by shape, is updated in place, and that row + // silently never appears. + render(view(['1', '2', '3', '4']), container); + assert.equal([...region.querySelectorAll('p')].length, 4, 'every row must render'); + assert.deepEqual([...region.querySelectorAll('p')].map((p) => p.textContent), ['1', '2', '3', '4']); +}); + // --- guard() --- test('guard: a throw during the commit does not blank the region forever', () => { diff --git a/website/app/docs/error-handling/page.ts b/website/app/docs/error-handling/page.ts index d678389a9..1ad20f1d9 100644 --- a/website/app/docs/error-handling/page.ts +++ b/website/app/docs/error-handling/page.ts @@ -127,7 +127,7 @@ export default function GlobalError({ error }: { error: Error }) {

A directive that throws mid-commit stays consistent

The component boundary above also covers watch(signal) and until(), which commit outside the update cycle, so a throw from either reaches renderError() rather than the window. It reaches the component whose template holds the binding, which is not always the element the binding sits inside: a watch() written between a child component's tags belongs to the parent that wrote it. asyncAppend / asyncReplace are not covered, in two ways: that path logs its own iteration throw and continues on purpose, and a watch() or until() nested inside a chunk it commits still reaches the window.

-

Beyond reporting the error, the directive's own state is left describing the DOM that actually exists, which is what makes the NEXT render correct. That matters because the failure is otherwise silent: the renders that expose it are fully valid and log nothing. The hole whose commit threw is marked so the next render re-applies it instead of skipping it as unchanged, which is what used to leave a region blank for good. repeat() additionally repairs its key map so it describes the DOM again, and the next render is an ordinary reconcile that repositions every row (the symptom was a permanently duplicated row); it deliberately does not rebuild the region, which would throw away the node identity keyed reconciliation exists to preserve. guard() records its new deps only once the commit succeeds, so a later render with those deps re-renders the region instead of skipping past one the throw had blanked; until() advances its resolved priority only after its commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it.

+

Beyond reporting the error, the directive's own state is left describing the DOM that actually exists, which is what makes the NEXT render correct. That matters because the failure is otherwise silent: the renders that expose it are fully valid and log nothing. The hole whose commit threw is marked so the next render re-applies it instead of skipping it as unchanged, which is what used to leave a 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 throw away the node identity they exist to preserve. repeat() re-unites its key map and repositions every row (the symptom was a permanently duplicated row). A plain .map() array splices back the part of its slot list the failed pass never reached, which is what a row whose template SHAPE changed in the throwing render needs, since that is the branch that inserts the replacement before removing what it replaced (the symptom 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 deps re-renders the region instead of skipping past one the throw had blanked; until() advances its resolved priority only after its commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it.

Tearing content back out is covered too, and it has to be, because a teardown has no next render to repair it. Unbinding a ref while a row is removed can never abort the removal of the rest of the list, 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. Without that, a throw part-way through left the row you had DELETED on screen, reordered the survivors, and let a later render that re-added the key reinsert the disposed instance. The cost is that a ref whose object value setter throws is swallowed on teardown, matching the ref callback, which was already swallowed everywhere (lit guards neither and propagates from both, so this is a deliberate divergence). It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches renderError().

Server action errors

From 04ef620a76afe9202693ca2cd218ef59971b1a12 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 16:25:59 +0530 Subject: [PATCH 6/6] fix: the empty-slot branch removes before it pushes, and the docs stop narrowing Three corrections to the array repair, none of which change the repair itself. The push-before-remove reorder was applied to the empty-slot branch for symmetry, and it is wrong there. The reorder exists to keep a slot that was already built and inserted tracked at a throw point, and the empty branch builds and inserts nothing, so pushing first only means a removal throw leaves a phantom empty slot at that index plus the old slot spliced in at the next one, shifting every later slot in a positional reconciler. Measured: rendering [null,'2','3'] over ['1','2','3'] with a refusing removal recovered to 2, X, 3 instead of X, 2, 3. It now removes first, like it used to. The residual note claimed a removal throw orphans nothing. It does: removeBetween takes the start marker first and early-returns for good once that marker is gone, so that row can never be removed afterwards and an empty render will not clear it. The comment now says so, matching what reconcileRepeat's catch already admits about its own equivalent. The tests and both doc surfaces described the trigger as a template SHAPE change. The destructive branch also takes an array that GREW past its old length (no old slot to compare against) and a slot whose KIND changed between text, template and empty, neither of which is a shape change. Growth reproduces the identical bug on the base branch, so that was a real coverage hole rather than only a wording one, and it now has a test. --- .agents/skills/webjs/references/components.md | 2 +- packages/core/src/render-client.js | 32 +++++++++++++------ .../rendering/directive-commit-throw.test.js | 28 +++++++++++++--- website/app/docs/error-handling/page.ts | 2 +- 4 files changed, 48 insertions(+), 16 deletions(-) diff --git a/.agents/skills/webjs/references/components.md b/.agents/skills/webjs/references/components.md index 02038574f..d91b4b04e 100644 --- a/.agents/skills/webjs/references/components.md +++ b/.agents/skills/webjs/references/components.md @@ -181,7 +181,7 @@ 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`${watch(sig)}`` 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.** 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 when an item's TEMPLATE SHAPE changes in the render that throws, 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. +**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. diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index 56a3a0d24..b70f284e0 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -2082,10 +2082,17 @@ function reconcileArray(part, value) { continue; } } else { - // Empty slot: drop any prior nodes that occupied this position. The - // push comes FIRST for the same reason as in the branch below. - next.push({ type: 'empty' }); + // 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; } @@ -2134,13 +2141,18 @@ function reconcileArray(part, value) { // for every row, which cancels an in-progress native drag and drops focus // and scroll. // - // Two residuals, both from the removal step itself rather than the - // bookkeeping. A throw out of `removeArrayItem` leaves the slot it was - // removing described one position later than it sits, costing that row - // its identity on the next render but orphaning nothing (and it takes a - // throwing DOM to reach at all, since the teardown it calls is total). - // Beyond that only `removeBetween` can throw, and it calls `removeChild` - // solely on nodes the renderer owns. + // 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; } diff --git a/packages/core/test/rendering/directive-commit-throw.test.js b/packages/core/test/rendering/directive-commit-throw.test.js index e2bac6b3d..138bc815b 100644 --- a/packages/core/test/rendering/directive-commit-throw.test.js +++ b/packages/core/test/rendering/directive-commit-throw.test.js @@ -323,10 +323,14 @@ function regionHTML(container) { return container.querySelector('div').innerHTML.replace(//g, ''); } -// The shape-changed branch is the destructive one: it inserts the replacement -// and removes the old slot BEFORE the walk can finish. A same-shape update -// touches only values, so it cannot reach this at all, which is why every -// case below changes an item's template SHAPE in the render that throws. +// The REPLACE branch is the destructive one: it inserts the replacement and +// removes the old slot BEFORE the walk can finish. An in-place update touches +// only values and cannot reach it. Three different things route there, and +// the cases below cover more than one, because narrowing this to "the +// template shape changed" would leave the others untested: the item's +// template shape changed, its slot KIND changed (text, template, empty), or +// the array GREW past the old length, where there is no old slot to compare +// against at all. test('array: a mid-walk throw does not strand a row on the next valid render', () => { const container = document.createElement('div'); @@ -352,6 +356,22 @@ test('array: a mid-walk throw does not strand a row on the next valid render', ( assert.equal(regionHTML(container), '1

2

'); }); +test('array: a GROWN array reaches the same branch, with no shape change at all', () => { + // Every item here is the same template shape, so nothing about this render + // is a "shape change". The new tail index simply has no old slot to reuse, + // which routes it to the same build-insert-remove branch. + const container = document.createElement('div'); + const view = (items) => html`
${items.map((v) => html`

${v}

`)}
`; + + render(view(['1']), container); + assert.throws(() => { render(view(['1', '2', poison]), container); }, /boom/); + + render(view(['1', '2', '3']), container); + assert.equal(regionHTML(container), '

1

2

3

'); + render(view([]), container); + assert.equal(regionHTML(container), ''); +}); + test('array: an EMPTY render after a throw leaves nothing behind', () => { const container = document.createElement('div'); const view = (items) => html`
${items.map((it) => ( diff --git a/website/app/docs/error-handling/page.ts b/website/app/docs/error-handling/page.ts index 1ad20f1d9..a6f2eab3b 100644 --- a/website/app/docs/error-handling/page.ts +++ b/website/app/docs/error-handling/page.ts @@ -127,7 +127,7 @@ export default function GlobalError({ error }: { error: Error }) {

A directive that throws mid-commit stays consistent

The component boundary above also covers watch(signal) and until(), which commit outside the update cycle, so a throw from either reaches renderError() rather than the window. It reaches the component whose template holds the binding, which is not always the element the binding sits inside: a watch() written between a child component's tags belongs to the parent that wrote it. asyncAppend / asyncReplace are not covered, in two ways: that path logs its own iteration throw and continues on purpose, and a watch() or until() nested inside a chunk it commits still reaches the window.

-

Beyond reporting the error, the directive's own state is left describing the DOM that actually exists, which is what makes the NEXT render correct. That matters because the failure is otherwise silent: the renders that expose it are fully valid and log nothing. The hole whose commit threw is marked so the next render re-applies it instead of skipping it as unchanged, which is what used to leave a 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 throw away the node identity they exist to preserve. repeat() re-unites its key map and repositions every row (the symptom was a permanently duplicated row). A plain .map() array splices back the part of its slot list the failed pass never reached, which is what a row whose template SHAPE changed in the throwing render needs, since that is the branch that inserts the replacement before removing what it replaced (the symptom 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 deps re-renders the region instead of skipping past one the throw had blanked; until() advances its resolved priority only after its commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it.

+

Beyond reporting the error, the directive's own state is left describing the DOM that actually exists, which is what makes the NEXT render correct. That matters because the failure is otherwise silent: the renders that expose it are fully valid and log nothing. The hole whose commit threw is marked so the next render re-applies it instead of skipping it as unchanged, which is what used to leave a 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 throw away the node identity they exist to preserve. repeat() re-unites its key map and repositions every row (the symptom was a permanently duplicated row). A plain .map() array splices back the part of its slot list the failed pass never reached, which is what a slot REPLACED rather than updated in place needs (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 symptom 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 deps re-renders the region instead of skipping past one the throw had blanked; until() advances its resolved priority only after its commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it.

Tearing content back out is covered too, and it has to be, because a teardown has no next render to repair it. Unbinding a ref while a row is removed can never abort the removal of the rest of the list, 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. Without that, a throw part-way through left the row you had DELETED on screen, reordered the survivors, and let a later render that re-added the key reinsert the disposed instance. The cost is that a ref whose object value setter throws is swallowed on teardown, matching the ref callback, which was already swallowed everywhere (lit guards neither and propagates from both, so this is a deliberate divergence). It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches renderError().

Server action errors