feat(virtual-core): add laneAssignmentMode option#1115
feat(virtual-core): add laneAssignmentMode option#11152wheeh wants to merge 11 commits intoTanStack:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 8496f50 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
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 |
There was a problem hiding this comment.
Pull request overview
Adds a new deferLaneAssignment option to @tanstack/virtual-core to support masonry-style layouts where lane assignment should be derived from measured sizes (via measureElement/resizeItem) rather than estimateSize, while keeping the existing “stable lanes via caching” behavior as the default.
Changes:
- Introduces
deferLaneAssignment?: boolean(defaultfalse) and conditionally skips lane caching until an item has been measured. - Adds tests covering deferred vs immediate lane caching behavior.
- Updates API docs and adds a changeset for release.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/virtual-core/src/index.ts | Adds deferLaneAssignment option + logic to skip caching lanes for unmeasured items |
| packages/virtual-core/tests/index.test.ts | Adds regression tests for deferred/immediate lane caching |
| docs/api/virtualizer.md | Documents new option and clarifies lane caching behavior |
| docs/api/virtual-item.md | Updates lane documentation to mention caching + deferral option |
| .changeset/loud-insects-itch.md | Publishes the change as a patch release |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/virtual-core/src/index.ts:732
deferLaneAssignmentis now part of the memo deps, but toggling it fromfalse→trueviasetOptionswill still keep any existinglaneAssignmentscached fromestimateSize, becausegetMeasurementspreferscachedLaneregardless ofdeferLaneAssignment. Consider detecting changes todeferLaneAssignment(similar tolanesChangedFlag) and clearinglaneAssignmentswhen it changes (at least when switching totrue) so the new mode can take effect at runtime.
this.options.deferLaneAssignment,
],
(count, paddingStart, scrollMargin, getItemKey, enabled, lanes, deferLaneAssignment) => {
const lanesChanged =
this.prevLanes !== undefined && this.prevLanes !== lanes
if (lanesChanged) {
// Set flag for getMeasurements to handle
this.lanesChangedFlag = true
}
this.prevLanes = lanes
this.pendingMeasuredCacheIndexes = []
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughAdded a new Virtualizer option Changes
Sequence Diagram(s)mermaid Client->>Virtualizer: setOptions(laneAssignmentMode) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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)
.changeset/loud-insects-itch.md (1)
1-5: Consider usingminorinstead ofpatchfor a new feature.Adding a new option (
deferLaneAssignment) is typically a minor version bump according to semver, since it introduces new functionality. While the default behavior is preserved, this may warrantminorrather thanpatchdepending on your project's versioning conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/loud-insects-itch.md around lines 1 - 5, The changeset currently marks '@tanstack/virtual-core' as a patch but adds a new feature (deferLaneAssignment); update the release type from "patch" to "minor" in this changeset so the version bump follows semver for new functionality—edit the changeset frontmatter replacing 'patch' with 'minor' for '@tanstack/virtual-core' (the changeset file is the one that declares the package and the release type).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.changeset/loud-insects-itch.md:
- Around line 1-5: The changeset currently marks '@tanstack/virtual-core' as a
patch but adds a new feature (deferLaneAssignment); update the release type from
"patch" to "minor" in this changeset so the version bump follows semver for new
functionality—edit the changeset frontmatter replacing 'patch' with 'minor' for
'@tanstack/virtual-core' (the changeset file is the one that declares the
package and the release type).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc9990a2-b929-4bb5-8fee-543be8140f8c
📒 Files selected for processing (5)
.changeset/loud-insects-itch.mddocs/api/virtual-item.mddocs/api/virtualizer.mdpackages/virtual-core/src/index.tspackages/virtual-core/tests/index.test.ts
…ignment Verify that lane assignments are cached progressively as items are measured and that earlier cached lanes remain stable when later items are measured. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@piecyk Hi, would love to know if you're open to this feature — and if the API shape needs changes (e.g. boolean vs string union), happy to adjust. |
@2wheeh Sorry for the late reply, and thanks for this! Let's replace deferLaneAssignment?: boolean with enum laneAssignmentMode?: 'estimate' | 'measured' (defaulting to 'estimate'). It better reflects the actual policy choice and gives us room to add more modes later without introducing another breaking API change. |
|
@2wheeh any updates on this? |
|
@piecyk Sorry, been busy working... could work tonight later! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/virtual-core/tests/index.test.ts (1)
310-325: Title says “default”, but the test sets'estimate'explicitly.Line 315 makes this an explicit-mode test, so it doesn’t catch regressions in default option wiring. Consider omitting
laneAssignmentModehere to validate the real default path.Suggested tweak
test("should cache lanes immediately when laneAssignmentMode is 'estimate' (default)", () => { const virtualizer = new Virtualizer({ count: 4, lanes: 2, estimateSize: () => 100, - laneAssignmentMode: 'estimate', getScrollElement: () => null, scrollToFn: vi.fn(), observeElementRect: vi.fn(), observeElementOffset: vi.fn(), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/virtual-core/tests/index.test.ts` around lines 310 - 325, The test sets laneAssignmentMode explicitly to 'estimate' but claims to verify the default; update the Virtualizer instantiation in the test (the object passed to new Virtualizer in index.test.ts) to omit the laneAssignmentMode property so the default branch is exercised (or alternatively set it to undefined), keeping the rest of the setup and the assertion against virtualizer['laneAssignments'].size unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/virtual-core/tests/index.test.ts`:
- Around line 310-325: The test sets laneAssignmentMode explicitly to 'estimate'
but claims to verify the default; update the Virtualizer instantiation in the
test (the object passed to new Virtualizer in index.test.ts) to omit the
laneAssignmentMode property so the default branch is exercised (or alternatively
set it to undefined), keeping the rest of the setup and the assertion against
virtualizer['laneAssignments'].size unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a7e41a8-0cf0-412e-8e39-2564610e7822
📒 Files selected for processing (5)
.changeset/loud-insects-itch.mddocs/api/virtual-item.mddocs/api/virtualizer.mdpackages/virtual-core/src/index.tspackages/virtual-core/tests/index.test.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/loud-insects-itch.md
- docs/api/virtual-item.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api/virtualizer.md
- packages/virtual-core/src/index.ts
|
@piecyk updated |
2wheeh
changed the title
|
View your CI Pipeline Execution ↗ for commit 54666bd
☁️ Nx Cloud last updated this comment at |
🎯 Changes
resolves #1114
Adds
deferLaneAssignmentoption to defer lane caching until items are measured viameasureElement.#1080 introduced lane assignment caching based on
estimateSizefor visual stability. However, this broke a core use case for masonry layouts where lane assignments should be based on actual measured sizes, not estimates.This PR adds a new option
deferLaneAssignment(default:false):false(default): Current behavior - lanes cached immediately fromestimateSizetrue: Lanes are calculated but not cached until first measurement, then cached based on actual sizes✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
Documentation
Tests
Chores