fix: show the correct group header when scrolled flush to its top#50
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Walkthrough将分组虚拟列表的粘性表头定位机制从基于行索引的 Changes粘性组头 headerRows → groupKeys 重构
估计代码审查工作量🎯 3 (Moderate) | ⏱️ ~20 minutes 可能相关的 PR
建议审查者
诗
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #50 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 8
Lines 209 211 +2
Branches 62 62
=========================================
+ Hits 209 211 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the sticky group header implementation in VirtualList to render the header inside a portal container rather than directly above the list items. It replaces the index-based headerRows tracking with a scroll-position-based groupKeys lookup. The review feedback highlights an edge case where scrolling above the first group header (such as during rubber-banding) incorrectly defaults to activating the first header. To resolve this, the reviewer suggests initializing the active index to -1, returning null early when no header is active, and adding a corresponding test case.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let left = 0; | ||
| let right = headerRows.length - 1; | ||
| let right = groupKeys.length - 1; | ||
| let activeIndex = 0; |
There was a problem hiding this comment.
When scrollTop is less than the first group header's top (e.g., during rubber-banding/elastic scrolling on iOS/macOS, or if there is header/padding space above the first group), findActiveHeaderIndex currently defaults to 0. This causes the first group's sticky header to remain visible at top: 0 even though the real header has not yet reached the top of the viewport, leading to a visual glitch (duplicate/detached header). We should initialize activeIndex to -1 to indicate that no header is active yet.
| let left = 0; | |
| let right = headerRows.length - 1; | |
| let right = groupKeys.length - 1; | |
| let activeIndex = 0; | |
| let left = 0; | |
| let right = groupKeys.length - 1; | |
| let activeIndex = -1; |
| const activeHeaderIdx = findActiveHeaderIndex( | ||
| groupKeys, | ||
| (groupKey) => getSize(groupKey).top, | ||
| scrollTop, | ||
| ); | ||
| const currGroupKey = groupKeys[activeHeaderIdx]; |
There was a problem hiding this comment.
Handle the case where activeHeaderIdx is -1 (no active header) by returning null early. This prevents rendering a redundant sticky header when the list is scrolled above the first group header, and avoids potential runtime errors when trying to access groupKeys[-1].
| const activeHeaderIdx = findActiveHeaderIndex( | |
| groupKeys, | |
| (groupKey) => getSize(groupKey).top, | |
| scrollTop, | |
| ); | |
| const currGroupKey = groupKeys[activeHeaderIdx]; | |
| const activeHeaderIdx = findActiveHeaderIndex( | |
| groupKeys, | |
| (groupKey) => getSize(groupKey).top, | |
| scrollTop, | |
| ); | |
| if (activeHeaderIdx === -1) { | |
| return null; | |
| } | |
| const currGroupKey = groupKeys[activeHeaderIdx]; |
| expect(title).toHaveBeenCalledWith('Group 2', baseItems.slice(3, 6)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add a test case to verify that the sticky header is not rendered when the list is scrolled above the first group header (e.g., during rubber-banding).
expect(title).toHaveBeenCalledWith('Group 2', baseItems.slice(3, 6));
});
it('returns null and does not render sticky header when scrolled above the first group header', () => {
const title = jest.fn().mockImplementation((key: React.Key) => (
<span data-testid="sticky-title">{String(key)}</span>
));
const info = createRenderInfo({
scrollTop: -10,
start: 0,
getSize: (key: React.Key) => ({ top: 0, bottom: 20 }),
});
const container = document.createElement('div');
document.body.appendChild(container);
const params: StickyHeaderParams<GroupedItem> = {
enabled: true,
group: {
key: (item) => item.group,
title,
},
groupKeys,
groupKeyToItems: baseItemsMap,
prefixCls: PREFIX_CLS,
listRef: createListRef(container),
};
render(<StickyHeaderTester params={params} info={info} />);
const stickyHeader = container.querySelector(
`.${PREFIX_CLS}-group-header-fixed`,
);
expect(stickyHeader).toBeNull();
});
});
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/VirtualList/useStickyGroupHeader.tsx`:
- Around line 72-75: The sticky header logic in useStickyGroupHeader is reading
listRef.current directly inside extraRender, which can be null on the first
RcVirtualList render and cause the header to miss its initial frame. Move the
container lookup into a synced state or layout effect, then have the Portal use
that stable container instead of accessing listRef.current during render; keep
the fix localized around useStickyGroupHeader and the Portal container handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4fe3e85-fe77-4b06-b8fe-1104d25bda81
📒 Files selected for processing (5)
assets/index.lesssrc/VirtualList/index.tsxsrc/VirtualList/useFlattenRows.tssrc/VirtualList/useStickyGroupHeader.tsxtests/hooks.test.tsx
When a group header lands exactly at the viewport top (which
scrollTo({ groupKey, align: 'top' }) always does), the sticky overlay
rendered the *previous* group's header pinned off-screen, blanking the
title. The active header was resolved from the virtual list's `start`
row index, whose `>=` range boundary includes the previous group's
trailing row at exact alignment — an off-by-one against the pixel-based
`top` positioning.
Resolve the active header by pixel offset (getSize(key).top <= scrollTop),
the same coordinate space as the positioning math. Add a regression test
for the exact-alignment boundary.
Incidental cleanup: drop the now-dead `rowIndex` and collapse the flatten
hook's `headerRows` to `groupKeys: K[]`.
… header The pixel-based selection compared header tops (summed item heights) against `scrollTop` with a strict `<=`. On a HiDPI screen the scroll offset snaps to a sub-pixel grid, so a trackpad scroll routinely rests ~0.5px short of a header's true top. The strict compare then resolved to the previous group and pinned its header off-screen, blanking the title — the same symptom as the original bug, but only reachable via real (non-integer) scrolling rather than `scrollTo`. Allow a 1px tolerance, which absorbs the rounding and stays far below the gap between consecutive headers. Add a regression test for the sub-pixel-short offset.
Remove comments explaining scrollTop behavior in useStickyGroupHeader.tsx.
c51f0c8 to
a8acbfb
Compare
|
@aojunhao123 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
2 participants
Before
After
Problem
In the grouped sticky list the sticky header can render blank — it shows the previous group's header pinned off-screen instead of the current group's title. Two distinct triggers, same symptom:
scrollTo({ groupKey, align: 'top' })(e.g.Scroll to G8thenScroll to G17) lands a header exactly at the viewport top.Root cause
1. Index vs pixel coordinate mismatch. The active header was chosen from the virtual list's
startrow index:but the overlay's
topis computed in pixels (getSize(...).top,scrollTop). rc-virtual-list derivesstartwithcurrentItemBottom >= offsetTop, so when a header sits flush at the top the previous group's trailing row (bottom ===scrollTop) is still counted asstart→ the lookup picks the previous group and thetopmath pins that stale header at a negative offset, clipping the title.2. Sub-pixel rounding. Selecting by pixel offset fixes (1), but
scrollTopis rounded to the device's sub-pixel grid while header tops come from summed item heights. On a Retina screen a trackpad scroll routinely rests ~0.5px below a header's true top, so a strictgetSize(top) <= scrollTopflips back to the previous group — the same blank, but only reachable via real (non-integer) scrolling rather thanscrollTo.Fix
last header with getSize(groupKey).top <= scrollTop— the same coordinate space as the positioning math, so selection andtopcan't disagree at an exact boundary.getSize(top) <= scrollTop + 1). 1px absorbs the rounding (the sub-pixel grid is <1px at any DPR) yet is far below the gap between consecutive headers, so it never skips a header. At the 1px-early switch point the incoming header'stopevaluates to exactly0(items_next + 1 > 0), so the tolerance can never re-introduce a negative/clipped header.Tests
startpoints at the previous group's last row. Verified red→green against the oldstart-based code.scrollTop0.5px short of a header top still resolves to the current group. Verified red→green (fails at strict<=, passes with the tolerance).top: 0.Incidental cleanup
Once selection no longer needs row indices,
rowIndexbecame dead — removed it, and collapsed the flatten hook'sheaderRows: { groupKey: K }[]togroupKeys: K[].🤖 Generated with Claude Code
Summary by CodeRabbit
新功能
问题修复