fix: a mid-commit throw strands a row in a plain .map() array - #1277
fix: a mid-commit throw strands a row in a plain .map() array#1277vivek7405 wants to merge 2 commits into
Conversation
8ab5fdf to
e6096d5
Compare
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fuzzed the repair itself before reading the prose (random old and new lists across all four slot kinds, random poison index): no recovery mismatches, and nothing survives a post-throw empty render. The splice and the consumed cursor are right. Three problems around it.
The one that matters is the push-before-remove reorder in the EMPTY-slot branch. The reorder earns its place in the branch below, where a slot has already been built and inserted and must not be left untracked. This branch builds and inserts nothing, so the reason does not apply, and it is not neutral: pushing first leaves a phantom empty slot at that index AND the old slot spliced in at the next one, which shifts every later slot in a POSITIONAL reconciler. That is a regression against base, taken for symmetry.
Second, the residual note says 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 and an empty render will not clear it. reconcileRepeat's catch admits exactly this about its own equivalent; this one denies it.
Third, four places narrow the trigger to a template SHAPE change. The same branch takes an array that GREW past its old length and a slot whose KIND changed. Growth reproduces the identical bug on base, so that was a coverage hole, not only a wording one.
| // 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 |
There was a problem hiding this comment.
Pushing before the removal is the right call one branch down, where something was already inserted. Nothing is inserted here, so on a removal throw this leaves a phantom empty slot at i and old[i] at i+1, shifting the whole tail. Removing first is correct for this branch.
There was a problem hiding this comment.
Fixed in f9b7b7cf. Removes first again, with a comment saying why this branch is deliberately not symmetric with the one below. Confirmed the ordering on the repro: [null,'2','3'] over ['1','2','3'] with a refusing removal now recovers to X, 2, 3 rather than 2, X, 3.
| // 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 |
There was a problem hiding this comment.
"orphaning nothing" is not true. removeBetween takes the start marker first and early-returns once it is gone, so a row whose removal refused can never be removed and survives an empty render.
There was a problem hiding this comment.
Fixed in f9b7b7cf. The note now states the residual instead of denying it, including that an empty render will not clear those nodes, and points at reconcileRepeat's catch as the same admission.
| return container.querySelector('div').innerHTML.replace(/<!--[^>]*-->/g, ''); | ||
| } | ||
|
|
||
| // The shape-changed branch is the destructive one: it inserts the replacement |
There was a problem hiding this comment.
A same-shape update cannot reach the branch, but a GROWN array can (no old slot at that index at all), and so can a slot changing KIND. Both reproduce the same bug on base, so scoping every case to a shape change leaves the rest uncovered.
There was a problem hiding this comment.
Fixed in f9b7b7cf. The comment names all three routes into that branch, both doc surfaces are widened the same way, and there is a growth-path test with no shape change anywhere in it. It reds when the catch is reverted, so it covers the bug rather than just the wording.
f9b7b7c to
4c47403
Compare
d1ed783 to
22a06d3
Compare
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.
4c47403 to
04ef620
Compare
1 participant
Closes #1250
Stacked on #1274 (which closes #1268). Base is
fix/repeat-leftover-removal-throw, so review the top commit; GitHub retargets this tomainonce #1274 merges. The dependency is real: this repair assumesremoveArrayItemdoes not throw, which is what #1274'sdisposeInstanceguard delivers.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 at65287383.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.