fix(data-table): Group fixes for virtualized and non virtualized table#791
fix(data-table): Group fixes for virtualized and non virtualized table#791paanSinghCoder wants to merge 18 commits intomainfrom
Conversation
`position: sticky` is bounded by its containing block. The sticky class was on the inner `VirtualHeaders` rowgroup, but the wrapping `<div ref={headerRef}>` (used for measurement) became the containing block and was only one header-row tall — so the header detached after ~one row of scroll.
Move `stickyHeader` onto the wrapper itself so its containing block becomes `.virtualTable` (full content height), restoring sticky behavior for the entire scroll range.
…te border-collapse and bottom border
`.virtualSectionHeader` and `.stickyGroupAnchor` had no font-size rule and inherited the document default (~16px), while the non-virtualized variant renders group headers at small (~12px) via the `<table>` font cascade. Added font-size and line-height tokens to both so they match.
- Hide natural section header for the current sticky group so it can't slide past the anchor. - New `useStickyGroupAnchor` hook (scrollTop-based current-group tracking, replaces overscan-affected index logic). - `groupHeaderHeight` default 32 — matches non-virtualized section header height. - Styling parity across both variants: color, letter-spacing, padding, 0.5px box-shadow. - Lock non-virtualized `.stickySectionHeader` height to 32px. - z-index/stacking fixes so column header's bottom border isn't covered in grouped mode.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request adds a sticky group anchor feature to the virtualized data-table component. A new Sequence Diagram(s)sequenceDiagram
participant Client
participant VirtualizedContent
participant useStickyGroupAnchor
participant VirtualRows
participant ScrollContainer
Client->>VirtualizedContent: render table with groupHeaderList & virtualizer
VirtualizedContent->>useStickyGroupAnchor: initialize hook(enabled, groupHeaderList, virtualizer, ref)
ScrollContainer->>VirtualizedContent: scroll event
VirtualizedContent->>useStickyGroupAnchor: recompute()
useStickyGroupAnchor->>VirtualizedContent: stickyGroupIndex
VirtualizedContent->>VirtualRows: render(hiddenGroupIndex=stickyGroupIndex)
VirtualRows->>VirtualRows: hide matching virtual row (visibility:hidden, aria-hidden)
VirtualizedContent->>Client: display anchored group via stickyGroupAnchor element
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/raystack/components/data-table/hooks/useStickyGroupAnchor.tsx (1)
50-58: ⚡ Quick win
measurementsCacheis an internal class field, not a documented public API
virtualizer.measurementsCacheis initialized in theVirtualizerconstructor asthis.measurementsCache = []but is not listed in the official TanStack Virtual v3 Virtualizer API surface. Reading it directly couples this hook to an implementation detail that could be renamed or made non-enumerable in a future minor release.The public
getVirtualItems()only returns the currently rendered window, which is insufficient here (group headers above the viewport won't appear). A safer alternative is to derive positions from the knownestimateSizesince group header heights are uniform and fixed (groupHeaderHeight):// Instead of virtualizer.measurementsCache[groupHeaderList[i].index] // derive the start offset from the virtualizer's public offset API: const offset = virtualizer.getOffsetForIndex(groupHeaderList[i].index)?.[0];Or accumulate positions from
estimateSizeat construction time (since group headers have a fixed height). UsingmeasurementsCacheworks today but carries a breakage risk across@tanstack/react-virtualpatch/minor bumps.Does `@tanstack/react-virtual` v3 Virtualizer expose a public API to get the pixel offset/start position of a specific item index, such as getOffsetForIndex or similar?🤖 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/raystack/components/data-table/hooks/useStickyGroupAnchor.tsx` around lines 50 - 58, The loop in useStickyGroupAnchor.tsx reads the internal virtualizer.measurementsCache (coupling to an internal API); replace that by deriving item start offsets via the virtualizer public API or by computing offsets from the known uniform groupHeaderHeight: inside the loop that iterates groupHeaderList use virtualizer.getOffsetForIndex(groupHeaderList[i].index) to obtain the start position (or compute start = index * groupHeaderHeight if you track/derive sizes) instead of accessing virtualizer.measurementsCache, and keep the same logic that compares start to scrollTop to set currentIdx.
🤖 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/raystack/components/data-table/data-table.module.css`:
- Around line 96-98: The z-index on .contentRoot thead is ineffective because
thead is not positioned; remove that rule and instead increase the z-index on
the positioned header cells so column headers stack above section headers—update
the .head rule (which already uses position: sticky) to a higher z-index (e.g.,
2) and ensure .stickySectionHeader keeps a lower z-index (e.g., 1) so header
cells render on top.
---
Nitpick comments:
In `@packages/raystack/components/data-table/hooks/useStickyGroupAnchor.tsx`:
- Around line 50-58: The loop in useStickyGroupAnchor.tsx reads the internal
virtualizer.measurementsCache (coupling to an internal API); replace that by
deriving item start offsets via the virtualizer public API or by computing
offsets from the known uniform groupHeaderHeight: inside the loop that iterates
groupHeaderList use virtualizer.getOffsetForIndex(groupHeaderList[i].index) to
obtain the start position (or compute start = index * groupHeaderHeight if you
track/derive sizes) instead of accessing virtualizer.measurementsCache, and keep
the same logic that compares start to scrollTop to set currentIdx.
🪄 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: 1fe7b1e0-c8db-45a8-8dbc-58229eb88035
📒 Files selected for processing (3)
packages/raystack/components/data-table/components/virtualized-content.tsxpackages/raystack/components/data-table/data-table.module.csspackages/raystack/components/data-table/hooks/useStickyGroupAnchor.tsx
| .contentRoot thead { | ||
| z-index: 2; | ||
| } |
There was a problem hiding this comment.
z-index on a non-positioned thead is a no-op — the stacking fix won't take effect
z-index only works on positioned elements (position: absolute, relative, fixed, or sticky) and flex items. The thead element has no position property set in this stylesheet (nor anywhere that would apply to it), so the z-index of a non-positioned element has no effect.
The intended stacking fix must be applied to the actual positioned elements. The individual column-header cells via .head carry position: sticky; z-index: 1, and .stickySectionHeader also uses z-index: 1 — same stacking level. Without z-index, elements stack in the order they appear in the DOM (the lowest one down at the same hierarchy level appears on top), so section-header cells (later in DOM) would paint on top of column-header cells when they share z-index: 1.
The correct fix is to raise the column header cells' own z-index, not the container:
🛡️ Proposed fix
-.contentRoot thead {
- z-index: 2;
-}
-
.head {
position: sticky;
top: 0;
- z-index: 1;
+ z-index: 2;
}🤖 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/raystack/components/data-table/data-table.module.css` around lines
96 - 98, The z-index on .contentRoot thead is ineffective because thead is not
positioned; remove that rule and instead increase the z-index on the positioned
header cells so column headers stack above section headers—update the .head rule
(which already uses position: sticky) to a higher z-index (e.g., 2) and ensure
.stickySectionHeader keeps a lower z-index (e.g., 1) so header cells render on
top.
…x not 28px) Previously used `--rs-space-8` (28px) thinking it was 32px. Actual 32px token is `--rs-space-9`. Now the non-virtualized section header height matches the virtualized `groupHeaderHeight = 32` default, and the sticky offset (32.5px) lands flush at the column header's bottom edge.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/raystack/components/data-table/components/virtualized-content.tsx (2)
342-359:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winA11y: the currently anchored group is never announced.
The sticky anchor is
aria-hidden='true'(correct, since it's a visual duplicate), and the natural in-flow group header at the same index is also rendered witharia-hidden='true'+visibility: 'hidden'(lines 119–124). For users opening the table, the very first group becomes the sticky one immediately, so its label/count is never reachable by assistive tech — only subsequent groups (which were briefly visible before being absorbed) have been announced.Consider keeping the natural row exposed to AT (drop
aria-hiddenon the hidden in-flow row, or expose the sticky anchor instead) so the current group context is always announced.🤖 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/raystack/components/data-table/components/virtualized-content.tsx` around lines 342 - 359, The anchored group header is visually duplicated and currently both the sticky anchor (in VirtualizedContent render: stickyGroupHeader && isGrouped && stickyGroup) and its original in-flow group header are hidden from assistive tech, so the first group is never announced; fix by keeping the visual sticky anchor aria-hidden='true' but ensure the in-flow group header (the original group row rendered elsewhere with visibility:'hidden') is not aria-hidden — remove or toggle aria-hidden on that in-flow group header so screen readers can announce the current group label/count (or alternatively mark the sticky anchor as exposed and remove aria-hidden there), updating the code paths that render the in-flow group header and the sticky anchor (stickyGroup, stickyGroup.showGroupCount, styles.stickyGroupAnchor) accordingly.
285-300:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
recomputeStickyGroupcallssetStateon every scroll event regardless of whether the active group changed.
handleVirtualScrollfires on every scroll tick and unconditionally callsrecomputeStickyGroup()whenstickyGroupHeaderis enabled. The hook'srecomputefunction performs a linear scan throughgroupHeaderListand always updatessetStickyGroupandsetStickyGroupIndex, even when the computed group hasn't actually changed. This causes unnecessary re-renders. Add an early return or compare against the previous group state before updating. Additionally,recompute's dependencies (groupHeaderList,virtualizer) may change frequently, causing the callback identity to shift andhandleVirtualScrollto be recreated as a result.🤖 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/raystack/components/data-table/components/virtualized-content.tsx` around lines 285 - 300, handleVirtualScroll currently calls recomputeStickyGroup on every scroll tick which triggers setStickyGroup/setStickyGroupIndex unconditionally; modify recomputeStickyGroup (the function referenced in this file) to compute the new group and compare it with the previous group/index (store previous in a ref) and return early without calling setState if nothing changed, and then memoize recomputeStickyGroup with useCallback (or keep a stable ref wrapper) so its identity doesn't change when groupHeaderList/virtualizer frequently update; this prevents unnecessary re-renders and avoids recreating handleVirtualScroll on each dependency change.
🧹 Nitpick comments (2)
packages/raystack/components/data-table/components/virtualized-content.tsx (2)
66-82: 💤 Low valueConfirm spread props can't override critical attributes.
Spreading
...restafterrole='row'andclassNameis fine for the current call site, which passes onlyaria-hiddenandstyle. Just be aware that any future caller passingrole,className, oronClickwill silently override these. If you want to harden this, place spread before the fixed attributes (or whitelist accepted props).🤖 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/raystack/components/data-table/components/virtualized-content.tsx` around lines 66 - 82, Spread props are currently applied after fixed attributes in VirtualGroupHeader, allowing callers to override role/className/onClick; change this by either spreading {...rest} before the fixed attributes or by explicitly extracting and sanitizing props (e.g., const { role, className, onClick, ...safeRest } = rest) then using safeRest while merging className with styles.virtualSectionHeader and hardcoding role='row' to prevent overrides; update VirtualGroupHeader to enforce role and merge className so callers can pass aria-hidden/style but cannot replace role/className/onClick.
272-283: 💤 Low value
anchorPixelHeightand virtualizerestimateSizeduplicate the same fallback expression.Both this line and
estimateSize(line 267) computegroupHeaderHeight ?? rowHeightfor group rows. Extracting a singlegroupHeaderPixelHeightconst and reusing it in both places (and passing it to the hook if it needs it) keeps them from drifting if the fallback rule ever changes.♻️ Suggested extraction
+ const groupHeaderPixelHeight = groupHeaderHeight ?? rowHeight; + const virtualizer = useVirtualizer({ count: rows.length, getScrollElement: () => scrollContainerRef.current, estimateSize: index => { const row = rows[index]; const isGroupHeader = row?.subRows && row.subRows.length > 0; - return isGroupHeader ? (groupHeaderHeight ?? rowHeight) : rowHeight; + return isGroupHeader ? groupHeaderPixelHeight : rowHeight; }, overscan }); - - const anchorPixelHeight = groupHeaderHeight ?? rowHeight; + const anchorPixelHeight = groupHeaderPixelHeight;🤖 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/raystack/components/data-table/components/virtualized-content.tsx` around lines 272 - 283, anchorPixelHeight and the virtualizer's estimateSize both compute the same fallback (groupHeaderHeight ?? rowHeight); extract a single constant (e.g., groupHeaderPixelHeight) assigned to groupHeaderHeight ?? rowHeight and use that constant everywhere instead of repeating the expression — update anchorPixelHeight to reference groupHeaderPixelHeight, update the estimateSize logic to reference groupHeaderPixelHeight, and if useStickyGroupAnchor requires the value, pass groupHeaderPixelHeight into its props; ensure you replace all occurrences of the duplicated expression in virtualized-content.tsx (including references around anchorPixelHeight, estimateSize, and the useStickyGroupAnchor call).
🤖 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.
Outside diff comments:
In `@packages/raystack/components/data-table/components/virtualized-content.tsx`:
- Around line 342-359: The anchored group header is visually duplicated and
currently both the sticky anchor (in VirtualizedContent render:
stickyGroupHeader && isGrouped && stickyGroup) and its original in-flow group
header are hidden from assistive tech, so the first group is never announced;
fix by keeping the visual sticky anchor aria-hidden='true' but ensure the
in-flow group header (the original group row rendered elsewhere with
visibility:'hidden') is not aria-hidden — remove or toggle aria-hidden on that
in-flow group header so screen readers can announce the current group
label/count (or alternatively mark the sticky anchor as exposed and remove
aria-hidden there), updating the code paths that render the in-flow group header
and the sticky anchor (stickyGroup, stickyGroup.showGroupCount,
styles.stickyGroupAnchor) accordingly.
- Around line 285-300: handleVirtualScroll currently calls recomputeStickyGroup
on every scroll tick which triggers setStickyGroup/setStickyGroupIndex
unconditionally; modify recomputeStickyGroup (the function referenced in this
file) to compute the new group and compare it with the previous group/index
(store previous in a ref) and return early without calling setState if nothing
changed, and then memoize recomputeStickyGroup with useCallback (or keep a
stable ref wrapper) so its identity doesn't change when
groupHeaderList/virtualizer frequently update; this prevents unnecessary
re-renders and avoids recreating handleVirtualScroll on each dependency change.
---
Nitpick comments:
In `@packages/raystack/components/data-table/components/virtualized-content.tsx`:
- Around line 66-82: Spread props are currently applied after fixed attributes
in VirtualGroupHeader, allowing callers to override role/className/onClick;
change this by either spreading {...rest} before the fixed attributes or by
explicitly extracting and sanitizing props (e.g., const { role, className,
onClick, ...safeRest } = rest) then using safeRest while merging className with
styles.virtualSectionHeader and hardcoding role='row' to prevent overrides;
update VirtualGroupHeader to enforce role and merge className so callers can
pass aria-hidden/style but cannot replace role/className/onClick.
- Around line 272-283: anchorPixelHeight and the virtualizer's estimateSize both
compute the same fallback (groupHeaderHeight ?? rowHeight); extract a single
constant (e.g., groupHeaderPixelHeight) assigned to groupHeaderHeight ??
rowHeight and use that constant everywhere instead of repeating the expression —
update anchorPixelHeight to reference groupHeaderPixelHeight, update the
estimateSize logic to reference groupHeaderPixelHeight, and if
useStickyGroupAnchor requires the value, pass groupHeaderPixelHeight into its
props; ensure you replace all occurrences of the duplicated expression in
virtualized-content.tsx (including references around anchorPixelHeight,
estimateSize, and the useStickyGroupAnchor call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e63d8c00-a190-4a92-b8d7-9a6bd1136be9
📒 Files selected for processing (3)
packages/raystack/components/data-table/components/virtualized-content.tsxpackages/raystack/components/data-table/data-table.module.csspackages/raystack/components/data-table/hooks/useStickyGroupAnchor.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/raystack/components/data-table/hooks/useStickyGroupAnchor.tsx
- packages/raystack/components/data-table/data-table.module.css
paanSinghCoder
changed the title
… for sticky anchors
1 participant
Aligns virtualized group anchor scroll behavior + styling with the non-virtualized table.
useStickyGroupAnchorhook (scrollTop-based current-group tracking via publicvirtualizer.getOffsetForIndex)..stickySectionHeaderheight to 32px to match virtualized.Routes
/examples/datatable— non-virtualized DataTable./examples/datatable-virtual— virtualized DataTable with grouping and infinite scroll.Test plan