Repository navigation
fix(svelte-virtual): preserve onChange across partial option updates - #1344
Conversation
🦋 Changeset detectedLatest commit: 65d055b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Retain onChange across option updates packages/svelte-virtual/src/index.ts, .changeset/tidy-scroll-callbacks.md |
createVirtualizerBase initializes and updates the retained callback. Partial updates that omit onChange keep its current value. |
Test callback and store updates packages/svelte-virtual/tests/index.test.ts, packages/svelte-virtual/package.json, packages/svelte-virtual/tsconfig.json, packages/svelte-virtual/vite.config.ts |
Tests cover preserving, replacing, and clearing onChange, as well as updates when no callback was initially supplied. The package adds a Vitest script and configures the test directory and TypeScript project files. |
Priority:
Estimated code review effort: 2 (Simple) | ~15 minutes
Change: Bug fix · Severity of issue fixed: Low
Possibly related PRs
- TanStack/virtual#1334: Preserves
onChangeduring partial option updates in the Lit controller adapter.
Suggested reviewers: yeungkc, piecyk
Merge Risk: 
65d05
-
The change preserves callbacks across partial option updates while retaining replacement and clearing behavior. No actionable merge-blocking issue remains; merge after normal checks pass.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: preserving the onChangecallback across partial option updates.Description check The description includes the required Changes, Checklist, and Release Impact sections. It explains the behavior, test coverage, issue reference, changeset, and the known incomplete pnpm run test:pr…Linked Issues check Issue #1343requires the existingonChangecallback to survive partialsetOptionsupdates for bothcreateVirtualizerandcreateWindowVirtualizer.packages/svelte-virtual/src/index.tsstores …Out of Scope Changes check The changes stay within issue #1343. The source change fixes callback retention. The tests, Vitest script, TypeScript inclusion, and Vite test configuration support regression coverage. The patch chan…Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Full details: Description check
-
Explanation
The description includes the required Changes, Checklist, and Release Impact sections. It explains the behavior, test coverage, issue reference, changeset, and the known incomplete
pnpm run test:prresult.
Full details: Linked Issues check
-
Explanation
Issue
#1343requires the existingonChangecallback to survive partialsetOptionsupdates for bothcreateVirtualizerandcreateWindowVirtualizer.packages/svelte-virtual/src/index.tsstores the user callback separately, retains it whenoptions.onChangeis omitted, and accepts explicit replacement orundefined. The parameterized tests cover both constructors, count-only updates, callback arguments, callback replacement, callback clearing, and store notifications.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue
#1343. The source change fixes callback retention. The tests, Vitest script, TypeScript inclusion, and Vite test configuration support regression coverage. The patch changeset documents the fix.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 3 files. (3 skipped: 3 unsupported.)
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
View your CI Pipeline Execution ↗ for commit 65d055b
☁️ Nx Cloud last updated this comment at |
piecyk
left a comment
piecyk
left a comment
There was a problem hiding this comment.
Thanks @clean99, nice catch. Keeping the user callback in its own variable is the right fix, since virtualizer.options.onChange is the store wrapper. I ran the new tests against main and the preserve and replace cases fail there, so they cover the bug. Also nice to finally have tests in svelte-virtual 🚀
Changes
setOptions({ count })stops the existingonChangecallback even though the setter accepts partial options. Omitted options should remain unchanged; state changes should still invoke the callback.Keep the user callback separate from the store-notification wrapper in
packages/svelte-virtual/src/index.ts. Explicit callback replacement andonChange: undefinedstill work. With only that source file reverted, both constructors fail the assertion thatmeasure()calls the retained callback exactly once after a count-only update.Fixes #1343
Checklist
pnpm run test:prpasses: the Svelte checks and all 6 affected example builds pass; root Knip reports the same 3 findings on unchanged main.Release Impact
Summary by CodeRabbit