fix(solid-virtual): fix [wip]#1216
Conversation
📝 WalkthroughWalkthrough
ChangesSolid Virtualizer State Update
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/solid-virtual/tests/reactivity.test.tsParsing error: "parserOptions.project" has been provided for Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/solid-virtual/tests/filter-items.test.ts (1)
6-51: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueGood regression test for index correctness; doesn't cover update granularity.
The assertions correctly validate
v.indexmapping intofiltered()across shrink/grow/empty scenarios. However, it only checks lengths and index-mapped values — it wouldn't catch a regression in per-item reactivity granularity (see concern raised inindex.tsxaboutsetVirtualItemsnow replacing array contents wholesale instead of diffing). Consider also asserting that unrelatedVirtualItemobject references remain stable across a filter change that doesn't affect their position/measurement, to guard the store's fine-grained-update contract going forward.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/solid-virtual/tests/filter-items.test.ts` around lines 6 - 51, The current test in filter-items.test.ts validates index mapping and counts, but it does not catch regressions in VirtualItem update granularity. Extend the test around createVirtualizer and getVirtualItems() to also compare object references before and after a filter change that preserves some items’ positions, asserting unchanged VirtualItem instances remain stable. Focus on the reactive update path exercised by setQuery and the array returned by getVirtualItems(), so the test guards the fine-grained store contract described in index.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/solid-virtual/tests/filter-items.test.ts`:
- Around line 6-51: The current test in filter-items.test.ts validates index
mapping and counts, but it does not catch regressions in VirtualItem update
granularity. Extend the test around createVirtualizer and getVirtualItems() to
also compare object references before and after a filter change that preserves
some items’ positions, asserting unchanged VirtualItem instances remain stable.
Focus on the reactive update path exercised by setQuery and the array returned
by getVirtualItems(), so the test guards the fine-grained store contract
described in index.tsx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99620b11-5b5b-4e27-a065-2ca9253d7c43
📒 Files selected for processing (3)
packages/solid-virtual/package.jsonpackages/solid-virtual/src/index.tsxpackages/solid-virtual/tests/filter-items.test.ts
|
@coderabbitai does the fix make sense in terms of logic and performance and what exact reactivity test is missing about your comment? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 1216 --repo TanStack/virtual --json title,body,comments --jq '.comments[] | select(.body | contains("reactivity")) | {author: .author.login, body: .body}'
echo "---REVIEW COMMENTS---"
gh api repos/TanStack/virtual/pulls/1216/comments --jq '.[] | {user: .user.login, path: .path, line: .line, body: .body}'Length of output: 673 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "---ISSUE COMMENTS---"
gh api repos/TanStack/virtual/issues/1216/comments --jq '.[] | {user: .user.login, body: .body}'
echo "---REVIEWS---"
gh api repos/TanStack/virtual/pulls/1216/reviews --jq '.[] | {user: .user.login, state: .state, body: .body}'Length of output: 12662 🏁 Script executed: #!/bin/bash
set -euo pipefail
fd . packages/solid-virtual/src -e tsx | xargs -I{} echo {}
echo "---"
cat -n packages/solid-virtual/src/index.tsx | sed -n '1,200p'Length of output: 4634
Does the fix make sense (logic + performance)?Logic: Yes, I believe it's correct. Performance: Should be roughly equivalent, if not marginally better — What reactivity test is missing?My earlier nitpick was about update granularity, not correctness of indices. The current
Both of these would pass even if A test that would catch this: trigger a filter/query update that changes the array length but leaves some early items' If you'd like, I can help draft that additional test case for ✏️ Learnings added
|
|
@coderabbitai can you give me a snippet for the test of what you mean please |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🤖 web_search executed:
Length of output: 10517
Here's what the Solid docs/GitHub discussion clarify:
So for This means the import { describe, expect, it } from 'vitest'
import { createRoot, createSignal } from 'solid-js'
import { unwrap } from 'solid-js/store'
import { createVirtualizer } from '../src/index'
describe('reactivity: unaffected slots keep stable references', () => {
it('does not recreate VirtualItem objects for slots whose data did not change', () => {
createRoot((dispose) => {
const data = Array.from({ length: 50 }, (_, i) => `item-${i}`)
const [filtered, setFiltered] = createSignal(data)
const virtualizer = createVirtualizer({
get count() {
return filtered().length
},
getScrollElement: () => document.createElement('div'),
estimateSize: () => 30,
overscan: 0,
})
const before = virtualizer.getVirtualItems()
const beforeRefs = before.map((item) => unwrap(item))
// Shrink the array; leaves the first visible rows' index/start/end/size untouched.
setFiltered(data.slice(0, 40))
const after = virtualizer.getVirtualItems()
const afterRefs = after.map((item) => unwrap(item))
const unaffectedCount = Math.min(beforeRefs.length, afterRefs.length)
for (let i = 0; i < unaffectedCount; i++) {
if (
beforeRefs[i].start === afterRefs[i].start &&
beforeRefs[i].end === afterRefs[i].end &&
beforeRefs[i].index === afterRefs[i].index
) {
expect(afterRefs[i]).toBe(beforeRefs[i])
}
}
dispose()
})
})
})This should slot in cleanly alongside your existing (•‿•) glad that cleared up cleanly ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/solid-virtual/tests/reactivity.test.ts`:
- Around line 12-19: The regression test in createVirtualizer is vulnerable to a
vacuous pass because getScrollElement returns a new unattached div with no
layout size, which can make the virtual range empty and skip all expects in the
loop. Update the test to use a stable scroll element reference with a non-zero
measured size (or stub its layout measurement) so the virtualizer computes a
meaningful range, and add an assertion in the test body to require that at least
one expectation actually runs before the test can pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85a9c965-1b5e-4a6c-a88c-e3ea0eee19da
📒 Files selected for processing (1)
packages/solid-virtual/tests/reactivity.test.ts
| const virtualizer = createVirtualizer({ | ||
| get count() { | ||
| return filtered().length | ||
| }, | ||
| getScrollElement: () => document.createElement('div'), | ||
| estimateSize: () => 30, | ||
| overscan: 0, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test can pass with zero assertions executed.
getScrollElement returns a fresh, unattached <div> with zero layout size, so the computed visible virtual-item range may end up empty (or all slots could end up "affected"). The subsequent loop only calls expect(...).toBe(...) inside a conditional guard (Lines 32-38), so if unaffectedCount is 0 or no index/start/end triple matches, the test finishes with no assertions ever run and still passes — silently defeating the purpose of this regression test.
Two independent fixes are needed:
- Give the scroll element a stable, non-zero measured size (or stub
getBoundingClientRect) and reuse the same element reference on each call. - Guard against a vacuous pass by requiring at least one assertion.
🐛 Proposed fix
+import { beforeEach, describe, expect, it } from 'vitest'
+
describe('reactivity: unaffected slots keep stable references', () => {
it('does not recreate VirtualItem objects for slots whose data did not change', () => {
createRoot((dispose) => {
const data = Array.from({ length: 50 }, (_, i) => `item-${i}`)
const [filtered, setFiltered] = createSignal(data)
+ const scrollElement = document.createElement('div')
+ Object.defineProperty(scrollElement, 'clientHeight', { value: 300 })
+ scrollElement.getBoundingClientRect = () =>
+ ({ height: 300, width: 300 }) as DOMRect
+
const virtualizer = createVirtualizer({
get count() {
return filtered().length
},
- getScrollElement: () => document.createElement('div'),
+ getScrollElement: () => scrollElement,
estimateSize: () => 30,
overscan: 0,
})
const before = virtualizer.getVirtualItems()
const beforeRefs = before.map((item) => unwrap(item))
+ expect(beforeRefs.length).toBeGreaterThan(0)
// Shrink the array; leaves the first visible rows' index/start/end/size untouched.
setFiltered(data.slice(0, 40))
const after = virtualizer.getVirtualItems()
const afterRefs = after.map((item) => unwrap(item))
const unaffectedCount = Math.min(beforeRefs.length, afterRefs.length)
+ let checkedAtLeastOne = false
for (let i = 0; i < unaffectedCount; i++) {
if (
beforeRefs[i].start === afterRefs[i].start &&
beforeRefs[i].end === afterRefs[i].end &&
beforeRefs[i].index === afterRefs[i].index
) {
+ checkedAtLeastOne = true
expect(afterRefs[i]).toBe(beforeRefs[i])
}
}
+ expect(checkedAtLeastOne).toBe(true)
dispose()
})
})
})Also applies to: 30-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/solid-virtual/tests/reactivity.test.ts` around lines 12 - 19, The
regression test in createVirtualizer is vulnerable to a vacuous pass because
getScrollElement returns a new unattached div with no layout size, which can
make the virtual range empty and skip all expects in the loop. Update the test
to use a stable scroll element reference with a non-zero measured size (or stub
its layout measurement) so the virtualizer computes a meaningful range, and add
an assertion in the test body to require that at least one expectation actually
runs before the test can pass.
|
I think this trying to fix same thing as in #1203? |
2 participants
🎯 Changes
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit