fix: a mid-commit throw strands a row in a plain .map() array - #1284
Conversation
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.
…p 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.
|
Review trail, carried over from #1277 This branch was reviewed on #1277 before GitHub closed that PR on a force-push. The content is identical, only the commit ids moved: The repair itself was fuzzed before the prose was read: 4000 trials over random old and new lists across all four slot kinds with a random poison index, no recovery mismatches, and nothing surviving a post-throw empty render. Three problems around it. 1. Fixed in 2. Fixed in 3. Four surfaces narrow the trigger to a template SHAPE change. The same destructive branch is reached by an array that GREW past its old length (no old slot at that index at all) and by a slot whose KIND changed between text, template and empty. Growth reproduces the identical bug on base, so this was a coverage hole rather than only a wording one. Fixed in What is not covered: the reviewer read the diff BEFORE those three fixes. |
1 participant
Closes #1250
Targets
main. #1274 (closing #1268) is already merged, which is what this depends on: the repair assumesremoveArrayItemdoes not throw, and that PR'sdisposeInstanceguard is what delivers it.Replaces #1277, which GitHub auto-closed when the branch was force-pushed to drop the now-squashed #1274 commits. Same branch, same work, no content difference; the review trail lives on #1277.
A plain
.map()array permanently stranded a row when an item's template SHAPE changed in the same render that threw.reconcileArrayaccumulated its replacement slot list locally and assignedstate.itemsonly after the whole walk, so a throw part-way discarded that 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 walksstate.items. Nothing was logged after the first throw.This is the same silent-corruption class #1172 fixed for
repeat(), in the one child-position reconciler it left alone. That premise (reconcileArray"leaves the DOM untouched on a throw") holds for the COMMON same-shape path, which touches only values. It does not hold for the shape-changed branch, which inserts the replacement and removes the old slot before the loop can finish.What changed
The walk, the shrink loop and the deferred commit are wrapped in a catch that splices the untouched tail of
oldonto whatnextaccumulated, then rethrows. The array analogue ofreconcileRepeat's catch: bookkeeping only, no attempt to unwind the partial DOM work, no teardown-and-rebuild. The invariant it guarantees, stated in the comment rather than an absolute: at any throw point every live node is described by exactly one slot, those belownext.lengthbeing the rebuilt or reused ones and the rest the part ofoldthe pass never reached. Index alignment survives because this reconciler is POSITIONAL.The splice boundary is a
consumedcursor, notnext.length. The shrink loop advances througholdwhilenextstops growing, so the two part company there. Splicing fromnext.lengthwould re-describe slots the shrink loop already removed, and the damage surfaces only later: a render that GREW the array would match a live value against a DETACHED slot with the samestrings, update it in place, and that row would silently never appear. There is a test for exactly that, and it is the one the cursor exists for.Both destructive branches push before they remove. A pure reordering on the success path, and it means a slot that has been built and inserted is never untracked at any throw point. This is not a substitute for the catch, which the filing left open: the reorder makes the failed POSITION atomic, while the corruption is that the whole list is committed late, so a throw at index 5 still discards indices 0 to 4. Both are needed.
Deliberately excluded
A teardown-and-rebuild of the region (measured on #1172 as keeping 0 of 3 node identities; a rebuild cancels an in-progress native drag and drops focus and scroll). Committing
state.itemsincrementally, which would leave later reads ofold[i]reading a partially rewritten array whilenextArrayAnchorscansoldforward. TheCOMMIT_FAILEDsentinel is complementary, not a substitute: it repairs the failed HOLE inside one instance, this repairsstate.items.Test plan
packages/core/test/rendering/directive-commit-throw.test.js): three new cases. A mid-walk throw followed by a valid render produces the exact list with no stranded row, and a second valid render is identical (so the recovery is not merely delayed); an EMPTY render after the throw leaves nothing behind, which is the sharpest probe since a slot tracked by nothing survives it; a throw in the SHRINK loop leaves no slot describing a detached row, proven by a later LONGER render rendering every row. 19/19 in the file, 396/396 acrosspackages/core/test/rendering.consumedcursor withnext.length(leaving the catch) reds ONLY the shrink-loop case, which is what proves that test earns its place. Proven at2842c96fanddd8b1473.packages/core/test/rendering/browser/directive-commit-throw.test.js): one new case for the identity fact linkedom cannot prove, that the row which did NOT change shape survives the recovery as the same element. Full run green on Chromium, Firefox and WebKit.render-client.jsis the client renderer, not a runtime-sensitive surface, and the parity hook does not match this path.Doc surfaces
.agents/skills/webjs/references/components.md: the parenthetical carve-out naming the plain array as still exempt is gone, and the paragraph now covers both list reconcilers.website/app/docs/error-handling/page.ts: that paragraph carried no carve-out at all, so it was overstating in exactly the way fix: make repeat/guard/watch/until commits atomic against a throw #1232 was corrected for. It now names the plain array rather than implying it by omission.