Conversation
Contributor
📝 WalkthroughWalkthroughAdds row-pinning support to the table implementation and examples. Introduces a new Vue 3 example component TableRowPinningExample.vue demonstrating pin/unpin actions and initializes 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. Documentation adds a "With row pinning" section (duplicated in the diff). Adds a test case exercising row pinning with top and bottom pinned IDs. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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}
Contributor
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: |
Contributor
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
| :: | ||
|
|
||
| ::tip | ||
| You can use the `row-pinning` prop to control the pinning state of the rows (can be binded with `v-model`). |
Contributor
There was a problem hiding this comment.
Fix wording on Line 519: use “bound” instead of “binded”.
This is a minor docs text quality issue in user-facing copy.
✏️ Proposed text fix
-You can use the `row-pinning` prop to control the pinning state of the rows (can be binded with `v-model`).
+You can use the `row-pinning` prop to control the pinning state of the rows (can be bound with `v-model`).📝 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
| You can use the `row-pinning` prop to control the pinning state of the rows (can be binded with `v-model`). | |
| You can use the `row-pinning` prop to control the pinning state of the rows (can be bound with `v-model`). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/docs/2.components/table.md` at 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".
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.
🔗 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