feat(Table): implement row pinning#6115
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:
📝 WalkthroughWalkthroughAdds end-to-end row-pinning support: a new Vue 3 example component (TableRowPinningExample.vue) implementing pin/unpin actions and initial top-pinned rows; updates runtime Table.vue to compute and render topRows, centerRows, and bottomRows and to use centerRows for virtualization keys and counts when pinned sections exist; updates a playground page to manage RowPinningState and bind v-model:row-pinning; inserts a documentation section describing row pinning (duplicated across docs); and adds table tests covering row pinning with and without virtualization. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/components/Table.vue (1)
628-645:⚠️ Potential issue | 🟠 MajorBottom-pinned rows are misplaced in virtualized mode.
At Line 644,
bottomRowsare rendered without virtualization offset. Since center rows are translated, bottom rows appear right after the rendered window instead of after the full center range.💡 Proposed fix
- <ReuseRowTemplate v-for="row in bottomRows" :key="row.id" :row="row" /> + <ReuseRowTemplate + v-for="row in bottomRows" + :key="row.id" + :row="row" + :style="virtualizer + ? { transform: `translateY(${virtualizer.getTotalSize() - renderedSize}px)` } + : undefined" + />🤖 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 628 - 645, Bottom-pinned rows are rendered without the virtualization offset so they appear immediately after the rendered window; fix by applying the same translateY offset equal to the virtualized center content height when virtualizer is present. In the template branch where virtualizer is truthy, render the bottomRows with ReuseRowTemplate but add a style prop that sets transform: `translateY(${virtualizer.getTotalSize()}px)` (or the virtualizer API that returns total center height) so bottomRows are positioned after the full virtualized center range; ensure you reference virtualizer, bottomRows, and ReuseRowTemplate when making the change.
🧹 Nitpick comments (1)
test/components/Table.spec.ts (1)
193-194: Add a virtualized row-pinning case to this matrix.This PR also changes the virtualized render path; adding one
virtualize + rowPinningscenario here would protect against regressions.🧪 Suggested test entry
- ['with row pinning', { props: { ...props, rowPinning: { top: ['2'], bottom: ['3'] } } }] + ['with row pinning', { props: { ...props, rowPinning: { top: ['2'], bottom: ['3'] } } }], + ['with row pinning and virtualization', { props: { ...props, virtualize: true, rowPinning: { top: ['2'], bottom: ['3'] } } }]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/Table.spec.ts` around lines 193 - 194, Add a new test matrix entry that combines virtualized rendering with row pinning to guard the changed virtualize path: update the matrix that currently contains entries like ['with body-bottom slot', { props, slots: { 'body-bottom': () => 'Body bottom slot' } }] and ['with row pinning', { props: { ...props, rowPinning: { top: ['2'], bottom: ['3'] } } }] by adding an entry such as ['with virtualize + row pinning', { props: { ...props, virtualize: true, rowPinning: { top: ['2'], bottom: ['3'] } } }] so the test suite exercises the virtualize code path together with rowPinning (ensure you reference the same props variable used in the matrix).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/app/components/content/examples/table/TableRowPinningExample.vue`:
- Around line 115-120: The UTable component usage is missing a data-slot
attribute; add a data-slot to the template element that renders <UTable> (for
example data-slot="table" or an appropriate name for this slot) so the line with
<UTable v-model:row-pinning="rowPinning" :data="data" :columns="columns"
class="flex-1" /> becomes <UTable data-slot="table"
v-model:row-pinning="rowPinning" :data="data" :columns="columns" class="flex-1"
/> (reference symbols: UTable, rowPinning, data, columns).
---
Outside diff comments:
In `@src/runtime/components/Table.vue`:
- Around line 628-645: Bottom-pinned rows are rendered without the
virtualization offset so they appear immediately after the rendered window; fix
by applying the same translateY offset equal to the virtualized center content
height when virtualizer is present. In the template branch where virtualizer is
truthy, render the bottomRows with ReuseRowTemplate but add a style prop that
sets transform: `translateY(${virtualizer.getTotalSize()}px)` (or the
virtualizer API that returns total center height) so bottomRows are positioned
after the full virtualized center range; ensure you reference virtualizer,
bottomRows, and ReuseRowTemplate when making the change.
---
Nitpick comments:
In `@test/components/Table.spec.ts`:
- Around line 193-194: Add a new test matrix entry that combines virtualized
rendering with row pinning to guard the changed virtualize path: update the
matrix that currently contains entries like ['with body-bottom slot', { props,
slots: { 'body-bottom': () => 'Body bottom slot' } }] and ['with row pinning', {
props: { ...props, rowPinning: { top: ['2'], bottom: ['3'] } } }] by adding an
entry such as ['with virtualize + row pinning', { props: { ...props, virtualize:
true, rowPinning: { top: ['2'], bottom: ['3'] } } }] so the test suite exercises
the virtualize code path together with rowPinning (ensure you reference the same
props variable used in the matrix).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ 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 (4)
docs/app/components/content/examples/table/TableRowPinningExample.vuedocs/content/docs/2.components/table.mdsrc/runtime/components/Table.vuetest/components/Table.spec.ts
commit: |
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 `@docs/content/docs/2.components/table.md`:
- Line 519: Update the docs text to fix wording: change the phrase "can be
binded with `v-model`" to "can be bound with `v-model`" in the sentence
describing the `row-pinning` prop; locate the sentence mentioning the
`row-pinning` prop and `v-model` (around the current line) and replace "binded"
with the correct past participle "bound".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbad7e4d-da18-49b9-b64d-e1c33e8cdb4c
📒 Files selected for processing (1)
docs/content/docs/2.components/table.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/runtime/components/Table.vue (1)
628-630: Minor: Inconsistent null handling between key and row prop.Line 628 uses optional chaining (
?.id) while line 630 uses non-null assertion (!). Since the virtualizer count is correctly set tocenterRows.value.length, both accesses should always be valid. Consider using non-null assertion consistently for the key as well:- <template v-for="(virtualRow, index) in virtualizer.getVirtualItems()" :key="centerRows[virtualRow.index]?.id"> + <template v-for="(virtualRow, index) in virtualizer.getVirtualItems()" :key="centerRows[virtualRow.index]!.id">🤖 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 628 - 630, The key uses optional chaining while the row prop uses a non-null assertion, causing inconsistent null handling; update the key expression to use a non-null assertion as well (i.e., replace centerRows[virtualRow.index]?.id with centerRows[virtualRow.index]!.id) so both accesses are consistent for the ReuseRowTemplate rendering driven by virtualizer.getVirtualItems() and centerRows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/docs/2.components/table.md`:
- Line 317: Update the heading "### With row pinning :badge{label=\"Soon\"
class=\"align-text-top\"}" to remove or replace the "Soon" badge since row
pinning is implemented; either remove the :badge snippet entirely or change it
to a version label such as :badge{label="4.1+" class="align-text-top"} so the
heading reflects availability (locate the "With row pinning" heading in the docs
content and edit the badge text accordingly).
---
Nitpick comments:
In `@src/runtime/components/Table.vue`:
- Around line 628-630: The key uses optional chaining while the row prop uses a
non-null assertion, causing inconsistent null handling; update the key
expression to use a non-null assertion as well (i.e., replace
centerRows[virtualRow.index]?.id with centerRows[virtualRow.index]!.id) so both
accesses are consistent for the ReuseRowTemplate rendering driven by
virtualizer.getVirtualItems() and centerRows.
🪄 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: a9dfceac-ae4e-4d0b-b8b0-0e330a920a40
⛔ 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 (5)
docs/app/components/content/examples/table/TableRowPinningExample.vuedocs/content/docs/2.components/table.mdplaygrounds/nuxt/app/pages/components/table.vuesrc/runtime/components/Table.vuetest/components/Table.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/components/Table.spec.ts
- docs/app/components/content/examples/table/TableRowPinningExample.vue
benjamincanac
left a comment
benjamincanac
left a comment
There was a problem hiding this comment.
@0xA1337 I've made a few improvements but otherwise it looks good thanks! 😊
I did try to implement sticky on rows when pinned but it added lots of complexity to calc the positions so I gave up for now.
2 participants
🔗 Linked issue
❓ Type of change
📚 Description
The
UTablecomponent currently ignores TanStack Table's row pinning state when rendering. While the pinning state is correctly managed internally (viav-model:row-pinningand the TanStack APIs), pinned rows are not rendered in their pinned positions -- they remain in their natural order.This is because the template uses
tableApi.getRowModel().rowswhich returns all rows in a flat list without any pinning awareness. TanStack Table expects consumers to render three separate row groups:getTopRows(),getCenterRows(), andgetBottomRows().Changes:
<tbody>templatedata-pinnedattribute on<tr>elements ("top","bottom", or omitted) for styling purposesrow.pin('top')📝 Checklist