Skip to content

fix(signals): repeat() handles disjoint window jumps (closes #2853) - #2854

Closed
yumemi-thomas wants to merge 2 commits into
solidjs:nextfrom
yumemi-thomas:fix/repeat-disjoint-window
Closed

fix(signals): repeat() handles disjoint window jumps (closes #2853)#2854
yumemi-thomas wants to merge 2 commits into
solidjs:nextfrom
yumemi-thomas:fix/repeat-disjoint-window

Conversation

@yumemi-thomas

Copy link
Copy Markdown

Closes #2853

Summary

repeat() / <Repeat> with a reactive from only handled window moves that overlap the previous window. Any disjoint jump — the everyday shape of a virtualized list, where dragging the scrollbar or jumping to an index moves from by more than count — broke three ways:

  1. Initial render with a nonzero from maps the whole prefixfrom: 10, count: 3 creates rows 0..12 (13 rows for a 3-row window).
  2. A forward jump larger than the window leaks every gap row — jumping 0 → 10 maps 3..12 and stores the gap rows (3..9) at negative _nodes indices, so nothing ever disposes them; their owners, effects and onCleanups stay live until the whole list is torn down. Each disjoint jump adds more (0 → 10 → 20 ⇒ 17 live scopes for a 3-row window).
  3. A backward disjoint jump crashes20 → 0 throws Cannot read properties of undefined (reading 'dispose'), the update aborts, and the list freezes on the old rows.

The rendered list always looks right (leaked rows never enter the DOM), so the leak is invisible without watching live scope counts. Repro: https://stackblitz.com/edit/solidjs-templates-de3jtq5s?file=src%2FApp.tsx

Distinct from #2767 (fixed by #2784 / d8921ac1): that covered the empty-window reset and overlapping backward slides, which still work. The disjoint geometries were never handled.

Root cause

updateRepeat (packages/solid-signals/src/map.ts) models every window move as "clear the end, shift the front, fill the difference" — valid only when old ∩ new ≠ ∅. The fill loop for (i = prevTo; i < to; i++) this._nodes[i - from] = … writes negative local indices for every i < from (symptoms 1–2), and the end-clear loop this._nodes[i - this._offset].dispose() reads this._nodes[negative] (undefined) on a backward disjoint jump (symptom 3).

Fix

Detect the disjoint case up front and replace the window wholesale:

if (from >= prevTo || to <= this._offset) {
  for (let i = 0; i < this._len; i++) this._nodes[i].dispose();
  this._nodes = new Array(newLen);
  this._mappings = new Array(newLen);
  for (let i = 0; i < newLen; i++)
    this._mappings[i] = runWithOwner(this._nodes[i] = createOwner(), () => this._map(from + i));
  this._offset = from;
  this._len = newLen;
  return;
}

That condition is the exact boolean complement of "the windows overlap" (from < prevTo && to > offset), so:

  • partial-overlap slides still take the existing preserving path and keep shared rows' identity/state — verified: [5,8) → [3,6) creates only rows 3,4 and preserves row 5;
  • only truly disjoint moves are replaced wholesale, which is unavoidable work (no shared rows to preserve);
  • it also subsumes the nonzero-first-render case (prevTo is 0 on first render).

The existing overlapping-slide branches (the #2784 fix) are untouched, and no loop-clamping is added — the guard makes the negative-index accesses unreachable rather than merely clamped.

Tests

tests/repeat.test.ts gains a "disjoint window jumps" block, all confirmed red-first on the unfixed source:

  • a forward jump larger than the window creates only the window's rows (live === 3, not 10)
  • repeated disjoint jumps do not accumulate live scopes (live === 3, not 17)
  • a backward disjoint jump maps the new window without crashing
  • initial render with a nonzero from maps only the window (3 created, not 13)

Solid 1.x note

Not applicable — repeat() / <Repeat> is a new 2.0 API with no 1.x counterpart (the closest 1.x pattern, <Index> over a sliced range, has no windowing bookkeeping and no equivalent failure mode).

Full disclosure: I wrote this with the help of an AI assistant (Claude Fable 5) and reviewed every line before pushing. All new tests were written first and confirmed failing on the unfixed code

@changeset-bot

changeset-bot Bot commented Jul 7, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ad738b4

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

This PR includes changesets to release 9 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

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 7, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 120 untouched benchmarks


Comparing yumemi-thomas:fix/repeat-disjoint-window (ad738b4) with next (cdbe95d)

Open in CodSpeed

@ryansolid

Copy link
Copy Markdown
Member

Excellent analysis — the "shift + fill is only valid when old ∩ new ≠ ∅" framing pins all three symptoms to one modeling assumption, and the guard being the exact boolean complement of the overlap condition is what makes this safe: partial-overlap slides can't accidentally take the replacement path, and disjoint moves have no shared rows to preserve anyway, so wholesale replacement is the minimum possible work. Also appreciated the red-first tests — we re-verified all four fail on the unfixed source before landing.

Checked the interaction with the fallback-dispose line above the guard (fallback is removed before the wholesale path runs, so no double-dispose) and the adjacent-window boundary (from === prevTo — half-open windows share nothing, replacement is correct). Landed as-is, changeset included, credited as co-author: 4e81e9c5. Ships in the next beta — thanks!

@ryansolid ryansolid closed this Jul 8, 2026
ryansolid added a commit that referenced this pull request Jul 8, 2026
updateRepeat modeled every window move as clear-end + shift-front + fill,
valid only when the old and new windows overlap. Disjoint moves walked
_nodes at negative local indices: forward jumps larger than the window
created and leaked every gap row (owners/effects alive, invisible to
dispose bookkeeping), first render with nonzero `from` mapped the whole
prefix, and backward disjoint jumps threw on _nodes[negative].dispose()
and froze the list. Detect disjointness up front (from >= prevTo ||
to <= offset — the exact complement of overlap) and replace the window
wholesale; overlapping slides (the #2784 fix) are untouched.

Co-authored-by: yumemi-thomas <yumemi-thomas@users.noreply.github.com>
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