Skip to content

fix(signals): guard reconcile's array paths against array/object shape changes - #2946

Merged
ryansolid merged 1 commit into
solidjs:nextfrom
brenelz:fix/reconcile-array-object-shape-guard
Jul 28, 2026
Merged

fix(signals): guard reconcile's array paths against array/object shape changes#2946
ryansolid merged 1 commit into
solidjs:nextfrom
brenelz:fix/reconcile-array-object-shape-guard

Conversation

@brenelz

@brenelz brenelz commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

The bug

reconcile()'s object diff refuses to recurse into a pair whose container kinds disagree — Array.isArray(previous) !== Array.isArray(next) replaces the slot instead of merging into it. The array paths reach the same recursion through keyedMatch and positional pairing, and never applied that rule: two keyless wrappables "match" because keyFn returns undefined for both, regardless of whether they're arrays or objects.

So an incoming array gets merged into a proxy whose target is an object. Array.isArray on a Proxy inspects the target, which is fixed at wrap time, so the slot is wrong forever after:

const [state, setState] = createStore({ list: [{ x: 1 }] });
setState(reconcile({ list: [[10, 20]] }, "id"));

Array.isArray(state.list[0]);       // false
JSON.stringify(state.list[0]);      // {"0":10,"1":20}

Spread, .map, <For each> and JSON.stringify all see an object. The reverse direction is worse — an array-shaped target receiving an object reads length off that object and the store presents [], silently dropping the data.

The fix

Five call sites shared the gap: the keyed prefix loop and the positional loop in both applyStateFast and applyStateSlow, plus applyArrayItem (which covers the keyed diff's trailing and moved slots). They now share a recursablePair() helper that folds the existing markRaw check together with the container-kind check, so a kind change replaces the slot by reference — exactly what the object diff and descendInto already do.

Net effect on the hot paths is neutral: recursablePair subsumes checks those sites already performed inline.

Tests

Four cases added to tests/store/reconcile.test.ts, covering both directions, the keyed-array trailing slot (applyArrayItem), and positional key: null merging. All four fail on next and pass with the fix.

Found by differential fuzzing (random trees reconciled against a plain-object oracle, checking both direct reads and effect-observed values). Full suite green: 19/19 turbo tasks, 1150 @solidjs/signals tests.

🤖 Generated with Claude Code

…e changes

The object diff refuses to recurse into a pair whose container kinds
disagree — `Array.isArray(previous) !== Array.isArray(next)` replaces the
slot instead of merging. The array paths reach the same recursion through
`keyedMatch` and positional pairing and never applied that rule: two
keyless wrappables "match" because `keyFn` returns undefined for both,
whatever kind they are.

The slot's proxy then stays the wrong kind forever, since `Array.isArray`
inspects the proxy target fixed at wrap time. An object slot receiving an
array enumerates as `{"0":..,"1":..}`; an array slot receiving an object
reads `length` off that object and presents an empty array, dropping the
data outright.

Route all five sites — the keyed prefix loop and the positional loop in
both applyStateFast and applyStateSlow, plus applyArrayItem — through a
shared `recursablePair()` that folds the existing markRaw check together
with the kind check, matching the object diff and `descendInto`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: b19d23a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages
Name Type
@solidjs/signals Patch
test-integration Patch
solid-js Patch
babel-preset-solid Patch
@solidjs/web Patch
@solidjs/html Patch
@solidjs/h Patch
@solidjs/universal Patch
@solidjs/element Patch
hackernews-spa-example Patch
hackernews-example Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codspeed-hq

codspeed-hq Bot commented Jul 28, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 7 untouched benchmarks
⏩ 117 skipped benchmarks1


Comparing brenelz:fix/reconcile-array-object-shape-guard (b19d23a) with next (63cd066)

Open in CodSpeed

Footnotes

  1. 117 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@ryansolid
ryansolid merged commit 7ab7614 into solidjs:next Jul 28, 2026
4 of 5 checks passed
@ryansolid

Copy link
Copy Markdown
Member

Merged — verified the diagnosis independently: descendInto (reconcile.ts:275) has always applied the container-kind rule, and keyedMatch really does let two keyless wrappables pair across kinds. Also confirmed both rewritten call sites are reached only with both sides wrappable (the keyedMatch loop guarantees it), so folding the checks into recursablePair is behaviour-preserving apart from the intended kind guard. All four new tests fail on next and pass with the fix; full suite and CodSpeed green.

ryansolid added a commit that referenced this pull request Jul 28, 2026
The scenario measured 12.59 KB and had already drifted past the 12.5 KB
cap before this batch of fixes. The overage is accumulated correctness
work on paths createStore always retains — reconcile's container-kind
guard (#2946), the projection derive-swap path (#2941), and the queue
traversal pass stamp (#2947) — not a shakeable feature. Restores the
~2% headroom the sibling scenarios carry.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants