feat (DataTable): add anchored header for groups#686
Open
paanSinghCoder wants to merge 2 commits intomainfrom
Open
feat (DataTable): add anchored header for groups#686paanSinghCoder wants to merge 2 commits intomainfrom
paanSinghCoder wants to merge 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)mermaid User->>Scroll: scroll event Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
paanSinghCoder
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/raystack/components/data-table/data-table.module.css (1)
172-179: Fixed header height assumption may be fragile.The
top: var(--rs-space-10)assumes a fixed table header height. If the header height varies (e.g., multi-line headers, different font sizes), the sticky section header may not align correctly under it. Consider whether this could be made more dynamic or documented as a constraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-table/data-table.module.css` around lines 172 - 179, .stickySectionHeader currently uses a fixed top: var(--rs-space-10) which breaks if the table header height changes; update the approach so the sticky section uses a dynamic header height: either introduce and use a dedicated CSS custom property (e.g. top: var(--table-header-height, var(--rs-space-10))) and ensure the table header sets --table-header-height to its computed height, or add a small script that measures the actual header element height (querySelector for the table header element) and sets the .stickySectionHeader style.top (or document.documentElement style.setProperty('--table-header-height', `${height}px`)) on mount and window resize; reference .stickySectionHeader and the new --table-header-height variable (or the header measurement logic) when applying the change.packages/raystack/components/data-table/components/virtualized-content.tsx (3)
341-354: Consider extracting duplicated group label rendering.The sticky anchor content (lines 347-352) duplicates the rendering logic from
VirtualGroupHeader(lines 74-78). Consider extracting the shared label+badge rendering into a reusable component:♻️ Suggested extraction
// Extract shared rendering function GroupLabelContent({ data }: { data: GroupedData<unknown> }) { return ( <Flex gap={3} align='center'> {data.label} {data.showGroupCount ? ( <Badge variant='neutral'>{data.count}</Badge> ) : null} </Flex> ); } // Then use in both places: // In VirtualGroupHeader: <div role='row' className={styles.virtualSectionHeader} style={style}> <GroupLabelContent data={data} /> </div> // In sticky anchor: {stickyGroupHeader && isGrouped && stickyGroup && ( <div role='row' className={styles.stickyGroupAnchor} style={{ top: headerHeight }}> <GroupLabelContent data={stickyGroup} /> </div> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-table/components/virtualized-content.tsx` around lines 341 - 354, The sticky group anchor duplicates the label+badge render logic found in VirtualGroupHeader; extract that shared markup into a small presentational component (e.g., GroupLabelContent) that accepts the grouped data shape and renders the label and optional Badge, then replace the inline JSX in both VirtualGroupHeader (use GroupLabelContent with the existing data prop) and the sticky anchor block (use GroupLabelContent with stickyGroup) so both places reference the same component and remove the duplicated code.
267-278: Performance:virtualizerin dependency array causes callback recreation on every render.
useVirtualizerreturns a new object reference each render, so includingvirtualizerin the dependency array meansupdateStickyGroupis recreated every render. This cascades tohandleVirtualScrolland theuseLayoutEffectat line 305-307, causing unnecessary work.Consider reading the virtualizer via a ref or accessing
getVirtualItems()directly within the scroll handler without memoizing this function separately:♻️ Suggested refactor using ref pattern
+ const virtualizerRef = useRef(virtualizer); + virtualizerRef.current = virtualizer; + const updateStickyGroup = useCallback(() => { if (!stickyGroupHeader || !isGrouped || groupHeaderList.length === 0) { setStickyGroup(null); return; } - const items = virtualizer.getVirtualItems(); + const items = virtualizerRef.current.getVirtualItems(); const firstIndex = items[0]?.index ?? 0; const current = groupHeaderList .filter(g => g.index <= firstIndex) .pop()?.data; setStickyGroup(current ?? null); - }, [stickyGroupHeader, isGrouped, groupHeaderList, virtualizer]); + }, [stickyGroupHeader, isGrouped, groupHeaderList]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-table/components/virtualized-content.tsx` around lines 267 - 278, The callback updateStickyGroup is being recreated every render because virtualizer (from useVirtualizer) is a new object reference each time; remove virtualizer from the dependency array and instead read virtualizer.getVirtualItems() via a stable ref or directly inside the scroll handler so updateStickyGroup only depends on stable values (stickyGroupHeader, isGrouped, groupHeaderList). Concretely: create a ref (e.g., virtualizerRef) and assign the virtualizer to it after initialization, update updateStickyGroup to call virtualizerRef.current?.getVirtualItems() rather than referencing virtualizer in the closure, and update any consumers (handleVirtualScroll and the useLayoutEffect) to use the ref so the callbacks are no longer recreated each render.
305-307: Minor: Redundant dependencies in effect.
groupHeaderListandisGroupedare already captured inupdateStickyGroup's dependency array, making them redundant here. If the callback recreation issue from the previous comment is addressed, you can simplify:✨ Suggested simplification
useLayoutEffect(() => { if (stickyGroupHeader) updateStickyGroup(); - }, [stickyGroupHeader, updateStickyGroup, groupHeaderList, isGrouped]); + }, [stickyGroupHeader, updateStickyGroup]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-table/components/virtualized-content.tsx` around lines 305 - 307, The useLayoutEffect dependency array includes redundant entries: remove groupHeaderList and isGrouped from the dependencies since they are already captured by the updateStickyGroup callback; keep stickyGroupHeader and updateStickyGroup in the useLayoutEffect dependency list (and ensure updateStickyGroup's own dependencies correctly include groupHeaderList and isGrouped) so the effect only runs when stickyGroupHeader or the callback changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/www/src/content/docs/components/datatable/props.ts`:
- Around line 39-43: Update the JSDoc for the grouping sticky-label prop in
apps/www/src/content/docs/components/datatable/props.ts so the human-readable
sentence matches the actual default: remove or correct the "(default)" token in
the phrase "When true (default)" and instead state the behavior with the
true/false semantics and keep the `@defaultValue` false unchanged; ensure the
description reads something like "When true, the current group label sticks
under the table header while scrolling (anchor group title). `@defaultValue`
false" so docs and implementation align.
---
Nitpick comments:
In `@packages/raystack/components/data-table/components/virtualized-content.tsx`:
- Around line 341-354: The sticky group anchor duplicates the label+badge render
logic found in VirtualGroupHeader; extract that shared markup into a small
presentational component (e.g., GroupLabelContent) that accepts the grouped data
shape and renders the label and optional Badge, then replace the inline JSX in
both VirtualGroupHeader (use GroupLabelContent with the existing data prop) and
the sticky anchor block (use GroupLabelContent with stickyGroup) so both places
reference the same component and remove the duplicated code.
- Around line 267-278: The callback updateStickyGroup is being recreated every
render because virtualizer (from useVirtualizer) is a new object reference each
time; remove virtualizer from the dependency array and instead read
virtualizer.getVirtualItems() via a stable ref or directly inside the scroll
handler so updateStickyGroup only depends on stable values (stickyGroupHeader,
isGrouped, groupHeaderList). Concretely: create a ref (e.g., virtualizerRef) and
assign the virtualizer to it after initialization, update updateStickyGroup to
call virtualizerRef.current?.getVirtualItems() rather than referencing
virtualizer in the closure, and update any consumers (handleVirtualScroll and
the useLayoutEffect) to use the ref so the callbacks are no longer recreated
each render.
- Around line 305-307: The useLayoutEffect dependency array includes redundant
entries: remove groupHeaderList and isGrouped from the dependencies since they
are already captured by the updateStickyGroup callback; keep stickyGroupHeader
and updateStickyGroup in the useLayoutEffect dependency list (and ensure
updateStickyGroup's own dependencies correctly include groupHeaderList and
isGrouped) so the effect only runs when stickyGroupHeader or the callback
changes.
In `@packages/raystack/components/data-table/data-table.module.css`:
- Around line 172-179: .stickySectionHeader currently uses a fixed top:
var(--rs-space-10) which breaks if the table header height changes; update the
approach so the sticky section uses a dynamic header height: either introduce
and use a dedicated CSS custom property (e.g. top: var(--table-header-height,
var(--rs-space-10))) and ensure the table header sets --table-header-height to
its computed height, or add a small script that measures the actual header
element height (querySelector for the table header element) and sets the
.stickySectionHeader style.top (or document.documentElement
style.setProperty('--table-header-height', `${height}px`)) on mount and window
resize; reference .stickySectionHeader and the new --table-header-height
variable (or the header measurement logic) when applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb45d7be-42cc-4ee9-9ce5-155e6f27122c
📒 Files selected for processing (9)
apps/www/src/app/examples/datatable/page.tsxapps/www/src/content/docs/components/datatable/index.mdxapps/www/src/content/docs/components/datatable/props.tspackages/raystack/components/data-table/__tests__/data-table.test.tsxpackages/raystack/components/data-table/components/content.tsxpackages/raystack/components/data-table/components/virtualized-content.tsxpackages/raystack/components/data-table/data-table.module.csspackages/raystack/components/data-table/data-table.tsxpackages/raystack/components/data-table/data-table.types.tsx
Comment on lines
+39
to
+43
| /** | ||
| * When true (default), the current group label sticks under the table header while scrolling (anchor group title). | ||
| * Applies to both Content and VirtualizedContent when grouping is enabled. | ||
| * @defaultValue false | ||
| */ |
Contributor
There was a problem hiding this comment.
Documentation inconsistency: description contradicts default value.
Line 40 says "When true (default)" but the @defaultValue on line 42 says false. The implementation in data-table.tsx confirms the default is false, so the description should be corrected.
📝 Proposed fix
/**
- * When true (default), the current group label sticks under the table header while scrolling (anchor group title).
+ * When true, the current group label sticks under the table header while scrolling (anchor group title).
* Applies to both Content and VirtualizedContent when grouping is enabled.
* `@defaultValue` false
*/
stickyGroupHeader?: boolean;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * When true (default), the current group label sticks under the table header while scrolling (anchor group title). | |
| * Applies to both Content and VirtualizedContent when grouping is enabled. | |
| * @defaultValue false | |
| */ | |
| /** | |
| * When true, the current group label sticks under the table header while scrolling (anchor group title). | |
| * Applies to both Content and VirtualizedContent when grouping is enabled. | |
| * `@defaultValue` false | |
| */ | |
| stickyGroupHeader?: boolean; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/www/src/content/docs/components/datatable/props.ts` around lines 39 -
43, Update the JSDoc for the grouping sticky-label prop in
apps/www/src/content/docs/components/datatable/props.ts so the human-readable
sentence matches the actual default: remove or correct the "(default)" token in
the phrase "When true (default)" and instead state the behavior with the
true/false semantics and keep the `@defaultValue` false unchanged; ensure the
description reads something like "When true, the current group label sticks
under the table header while scrolling (anchor group title). `@defaultValue`
false" so docs and implementation align.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
feat (data-table): add anchored group header. Toggled with
stickyGroupHeader. DefaultfalseTest it here: https://apsara-git-feat-anchored-group-header-raystack.vercel.app/examples/datatable
Screen.Recording.2026-03-10.at.4.36.37.PM.mov
Type of Change
How Has This Been Tested?
[Describe the tests that you ran to verify your changes]
Checklist:
Screenshots (if appropriate):
[Add screenshots here]
Related Issues
[Link any related issues here using #issue-number]
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes