Repository navigation
fix(virtual-core): account for scrollMargin when deciding the viewport is at the end - #1340
Conversation
…t is at the end `getVirtualDistanceFromEnd` subtracted a scroll offset measured from the top of the scroll content from a total that starts `scrollMargin` into it, so the end check read the viewport `scrollMargin` px closer to the end than it is. With `anchorTo: 'end'` that pulled a reader sitting inside the margin back down when an item grew, while `getDistanceFromEnd()` and `isAtEnd()`, which use the element's own max scroll, correctly reported them as away from the end. Lift the total into the element's space before subtracting. `scrollMargin` defaults to 0, so lists that do not set it are unaffected, and an end-pinned viewport still gets pinned. Co-Authored-By: Claude Opus 5 <[email protected]>
🦋 Changeset detectedLatest commit: 5d760dc The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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 |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
End-distance calculation and regression coverage packages/virtual-core/src/index.ts, packages/virtual-core/tests/anchor-end.test.ts, .changeset/scroll-margin-end-check.md |
getVirtualDistanceFromEnd adds scrollMargin before calculating the distance and still clamps the result to zero. Tests cover streaming growth when a reader is 100px from the end and when the viewport is pinned to the end. The changeset records a patch release for @tanstack/virtual-core. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested reviewers: piecyk
Merge Risk: 
5d760
-
The change is ready to merge; the intended nonzero-scroll-margin behavior is covered without identified regressions.
Pre-merge checks: 
-
Check name Status Explanation Title check The title follows the conventional commit format and names the primary change: getVirtualDistanceFromEnd now accounts for scrollMargin when deciding whether the viewport is at the end. It matches the … Description check The description covers the change and its motivation, linked issue #1339, a worked example table, the test changes, and the validation run. It fills in the template's Changes, Checklist, and Release I…Linked Issues check Issue #1339is directly linked and requiresscrollMargin-aware end detection while preserving end pinning and thescrollMargin: 0behavior.getVirtualDistanceFromEndnow adds `this.options.scrol…Out of Scope Changes check The changed source implements the correction requested by issue #1339. The added tests verify the affected nonzero-margin behavior and preserve end pinning. The changeset documents the published packa…Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 … Full details: Title check
-
Explanation
The title follows the conventional commit format and names the primary change: getVirtualDistanceFromEnd now accounts for scrollMargin when deciding whether the viewport is at the end. It matches the code change in packages/virtual-core/src/index.ts and the added tests, so it is specific and accurate.
Full details: Description check
-
Explanation
The description covers the change and its motivation, linked issue
#1339, a worked example table, the test changes, and the validation run. It fills in the template's Changes, Checklist, and Release Impact sections. The Release Impact section checks the changeset box, and the raw summary confirms a changeset was added. The description does not follow the template's emoji headings exactly, but all required information is present.
Full details: Linked Issues check
-
Explanation
Issue
#1339is directly linked and requiresscrollMargin-aware end detection while preserving end pinning and thescrollMargin: 0behavior.getVirtualDistanceFromEndnow addsthis.options.scrollMarginbefore subtracting the viewport size and scroll offset. The result remains clamped to zero. The change therefore aligns the internal distance with the element-space distance used by the public end checks. The added tests cover a reader 100px from the end with a 150px margin and an end-pinned viewport with the same margin. Existing zero-margin behavior remains covered by the existing tests.
Full details: Out of Scope Changes check
-
Explanation
The changed source implements the correction requested by issue
#1339. The added tests verify the affected nonzero-margin behavior and preserve end pinning. The changeset documents the published package fix. No unrelated product behavior or unrelated files appear in the whole-PR diff.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 ESLint
-
If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.
packages/virtual-core/tests/anchor-end.test.ts
-
Parsing error: "parserOptions.project" has been provided for
@typescript-eslint/parser.
The file was not found in any of the provided project(s): packages/virtual-core/tests/anchor-end.test.ts
-
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.
piecyk
left a comment
piecyk
left a comment
There was a problem hiding this comment.
Thanks @KooYS, nice catch and a really clear write-up. The math checks out, the total is in list space and the offset is in element space, so adding scrollMargin back makes it line up with getDistanceFromEnd(). I also ran the new test on main and it fails there, so it covers the bug. LGTM 🚀
|
View your CI Pipeline Execution ↗ for commit 5d760dc
☁️ Nx Cloud last updated this comment at |
🎯 Changes
Fixes #1339.
getVirtualDistanceFromEndsubtracts two values measured from different origins:_getLayoutTotalSize()is the sizer, which isend - scrollMargin + paddingEndand therefore startsscrollMargininto the scroll content.getScrollOffset()is measured from the top of that content. The result reads the viewportscrollMarginpx closer to the end than it is, soresizeItem'swasAtEndfires for a reader who is still inside the margin and an item growing below the fold pulls them down.getDistanceFromEnd()andisAtEnd()are computed from the element's own max scroll and are correct, so today they report "not at the end" for the same viewport the internal check treats as pinned.The change lifts the total into the element's space before subtracting.
scrollMargindefaults to 0, so a list that does not set it is unaffected. Reading the intent the other way round gives the same expression: if the distance is meant to be from the end of the list rather than of the element, the scroll offset is the value that needs the margin taken off it.With
scrollMargin: 300and the defaultscrollEndThresholdof 1, growing the last row:isAtEnd()Tests.
createChatVirtualizertakes ascrollMargin, which also moves the mock element'sscrollHeight, and two cases sit next to the existing end-anchor tests: a reader 100px above the end with a 150px margin is left alone, which fails onmain, and an end-pinned viewport with the same margin still gets its 70px adjustment, which passes before and after.✅ Checklist
pnpm run test:pr.For
virtual-core:test:lib440 passing and 1 skipped,test:types,test:eslint,build. At the root:test:sherif,test:docs,test:knip.test:builddid not run on this machine, it fails the same way on a cleanmainhere becausepublintshells out to the systempnpm. I did not run the Playwright e2e.🚀 Release Impact
🤖 Generated with Claude Code
Summary by CodeRabbit