feat(table): sticky header footer in virtualized mode#6217
feat(table): sticky header footer in virtualized mode#6217ChronicStone wants to merge 7 commits intonuxt:v4from
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 ignored due to path filters (2)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated docs to state Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/Table.vue`:
- Around line 625-632: The key/prop mismatch arises because virtualRow.index may
be out of bounds for centerRows: align behavior by skipping rendering when the
row is undefined—inside the v-for over virtualizer.getVirtualItems(), compute
const row = centerRows[virtualRow.index] and only render ReuseRowTemplate when
row exists (i.e., guard on row) so both the :key and :row prop use the same
non-null value; reference virtualizer.getVirtualItems(), centerRows,
virtualRow.index, ReuseRowTemplate, virtualPaddingTop and virtualPaddingBottom
to locate the block and add the guard that prevents rendering when row is
undefined.
🪄 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: b1c6c703-c842-4f02-9d59-a5e8d3e2dce1
📒 Files selected for processing (2)
docs/content/docs/2.components/table.mdsrc/runtime/components/Table.vue
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
PR.md (1)
18-18: Optional wording polish for readability.“works cleanest” reads slightly awkwardly; “works the cleanest” or “is the cleanest approach” is smoother.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PR.md` at line 18, Edit the sentence in PR.md that currently reads "The approach I found that works cleanest is switching from transform-based row positioning to the spacer-row strategy." and replace "works cleanest" with a smoother alternative such as "works the cleanest" or "is the cleanest approach" to improve readability; ensure the rest of the sentence remains unchanged (keeping references to `translateY`, `<tr>`, and `<tbody>` intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PR.md`:
- Around line 20-23: The fenced code block containing the expressions for
paddingTop and paddingBottom lacks a language identifier; update the markdown by
adding a language tag (e.g., ```text) to the opening fence so the block is
annotated (the code showing paddingTop = virtualItems[0].start and paddingBottom
= totalSize - virtualItems[last].end should be wrapped with ```text ... ```).
This will satisfy the MD040 lint rule without changing the content of the
expressions or variable names (paddingTop, paddingBottom, virtualItems,
totalSize, last).
---
Nitpick comments:
In `@PR.md`:
- Line 18: Edit the sentence in PR.md that currently reads "The approach I found
that works cleanest is switching from transform-based row positioning to the
spacer-row strategy." and replace "works cleanest" with a smoother alternative
such as "works the cleanest" or "is the cleanest approach" to improve
readability; ensure the rest of the sentence remains unchanged (keeping
references to `translateY`, `<tr>`, and `<tbody>` intact).
🪄 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: eb996ead-9350-40fb-aeb4-cfd4552814ca
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/Table-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Table.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
PR.md
commit: |
|
Thanks ! It's been 2 days I'm trying to find a workaround that works good. Every try I get problems. By the way for the dividers, this workaround worked for me: adding this to UTable |
howwohmm
left a comment
howwohmm
left a comment
There was a problem hiding this comment.
clean approach — the spacer-row strategy is the right way to enable sticky headers with virtualization. TanStack Virtual's own examples use this pattern.
one thing worth checking: the spacer `` elements are self-closing with no `` children. while this is technically valid HTML ("zero or more" in the content model), some browsers may not reliably apply `height` to a childless `` since there's no cell to drive the row's block formatting. adding a single `` inside each spacer would be safer for cross-browser consistency.
also noticed there's no test case for the sticky + virtualize combination — the existing "renders with virtualize correctly" snapshot doesn't pass `sticky: true`. since that's the feature being unlocked, a dedicated test would be good.
ChronicStone
force-pushed
the
features/sticky-virtual-table
branch
from
331ffaa to
a988c50
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/runtime/components/Table.vue (1)
626-629:⚠️ Potential issue | 🟡 MinorGuard
centerRows[virtualRow.index]before rendering.
:keyalready treats this lookup as nullable, but:row="centerRows[virtualRow.index]!"still dereferences it. A transient out-of-bounds virtual item during row-model updates will blow up insideReuseRowTemplate.🛡️ Suggested guard
- <template v-for="virtualRow in virtualizer.getVirtualItems()" :key="centerRows[virtualRow.index]?.id"> + <template v-for="virtualRow in virtualizer.getVirtualItems()" :key="centerRows[virtualRow.index]?.id ?? virtualRow.index"> <ReuseRowTemplate + v-if="centerRows[virtualRow.index]" :row="centerRows[virtualRow.index]!" :style="{ height: `${virtualRow.size}px` }" /> </template>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Table.vue` around lines 626 - 629, The ReuseRowTemplate currently dereferences centerRows[virtualRow.index] without guarding, which can throw if virtualRow.index is temporarily out-of-bounds; update the v-for block that iterates virtualizer.getVirtualItems() to first resolve the row (e.g., const row = centerRows[virtualRow.index]) and render ReuseRowTemplate only when row is defined (use a v-if or skip that virtualRow), keep the :key using the guarded value and pass the non-null row to the :row prop (remove the non-null assertion) so ReuseRowTemplate never receives an undefined row.
🧹 Nitpick comments (1)
src/runtime/components/Table.vue (1)
279-285: Please add asticky + virtualizeregression case.This flips the feature gate, but
test/components/Table.spec.tsstill coversstickyandvirtualizeseparately. Adding a combined case withcolumnswould also exercise the newly affectedtfootpath.🧪 Suggested coverage
renderEach(Table, [ // Props ['with data', { props }], ['without data', {}], ['with empty', { props: { empty: 'There is no data' } }], ['with caption', { props: { ...props, caption: 'Table caption' } }], ['with columns', { props: { ...props, columns } }], ['with sticky', { props: { ...props, sticky: true } }], + ['with sticky and virtualize', { props: { ...props, columns, sticky: true, virtualize: true } }], ['with loading', { props: { ...props, loading: true } }],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Table.vue` around lines 279 - 285, Add a regression test to test/components/Table.spec.ts that exercises the combined sticky + virtualize scenario (e.g., a new "sticky + virtualize" it/test) by mounting the Table component with props sticky: true and virtualize: true, providing a non-empty columns array and a tfoot slot (to hit the tfoot path), and asserting that the tfoot is rendered and that sticky-related classes/attributes and virtualization behavior are present; reference the Table component mount helpers used elsewhere in the file to keep the pattern consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/runtime/components/Table.vue`:
- Around line 626-629: The ReuseRowTemplate currently dereferences
centerRows[virtualRow.index] without guarding, which can throw if
virtualRow.index is temporarily out-of-bounds; update the v-for block that
iterates virtualizer.getVirtualItems() to first resolve the row (e.g., const row
= centerRows[virtualRow.index]) and render ReuseRowTemplate only when row is
defined (use a v-if or skip that virtualRow), keep the :key using the guarded
value and pass the non-null row to the :row prop (remove the non-null assertion)
so ReuseRowTemplate never receives an undefined row.
---
Nitpick comments:
In `@src/runtime/components/Table.vue`:
- Around line 279-285: Add a regression test to test/components/Table.spec.ts
that exercises the combined sticky + virtualize scenario (e.g., a new "sticky +
virtualize" it/test) by mounting the Table component with props sticky: true and
virtualize: true, providing a non-empty columns array and a tfoot slot (to hit
the tfoot path), and asserting that the tfoot is rendered and that
sticky-related classes/attributes and virtualization behavior are present;
reference the Table component mount helpers used elsewhere in the file to keep
the pattern consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f237faac-0996-4b36-874d-269adbeab8dc
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/Table-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Table.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
docs/content/docs/2.components/table.mdsrc/runtime/components/Table.vue
✅ Files skipped from review due to trivial changes (1)
- docs/content/docs/2.components/table.md
|
Hi @howwohmm thanks for the feedback, I took that solution straight out of tanstack table examples & github issues, I explored other solutions but this is what worked best. I adressed the two things you mentionned, childless and missing test case, everything should be good now 😄 |
3 participants
🔗 Linked issue
Resolves #5464
❓ Type of change
📚 Description
The
stickyprop was explicitly disabled whenvirtualizewas on, and the docs listed it as unsupported.The approach I found that works cleanest is switching from transform-based row positioning to the spacer-row strategy. Instead of applying
translateYto each virtual row, two empty<tr>elements act as spacers at the top and bottom of<tbody>:Rows render at their natural position in the flow with no transforms, which means
position: stickyon<thead>and<tfoot>just works. The outer<div>height wrapper and therenderedSizecomputed (used only for the old tfoot transform) are also gone.📝 Checklist