Repository navigation
feat: Data table row selection - #358
Merged
Merged
Conversation
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds row-selection support across the data table stack: new props (enableRowSelection, rowSelection, onRowSelectionChange), controlled/uncontrolled wiring, header + per-row checkboxes, export of RowSelectionState, QueryResultPanel forwarding, and minor UI/formatting adjustments and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant QRP as QueryResultPanel
participant DTP as DataTablePaginated
participant RT as `@tanstack/react-table`
participant UI as Checkbox UI
QRP->>DTP: render with (enableRowSelection, rowSelection?, onRowSelectionChange?)
DTP->>RT: initialize table(state: { rowSelection }, onStateChange: handleRowSelectionChange)
RT-->>DTP: provide rows + selection state
DTP->>UI: render header checkbox and per-row checkboxes based on state
UI-->>DTP: user toggles checkbox (events stopPropagation)
DTP->>RT: update selection (table.setRowSelection) or call onRowSelectionChange
DTP->>QRP: if controlled, bubble selection changes via onRowSelectionChange prop
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 |
…ow-selection Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
ilyabo
marked this pull request as ready for review
February 20, 2026 12:24
![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: 3
🧹 Nitpick comments (3)
packages/data-table/src/DataTableArrowPaginated.tsx (1)
11-19: Consider forwarding row selection props throughDataTableArrowPaginated.
DataTableArrowPaginatedis a publicly exported component and the primary API for consumers working witharrow.Tabledata. Now thatDataTablePaginatedhas first-classenableRowSelection/rowSelection/onRowSelectionChangesupport, omitting them here means Arrow-table consumers must drop down toDataTablePaginated + useArrowDataTabledirectly to get selection, which undercuts the convenience wrapper's value.♻️ Proposed change
+import {RowSelectionState} from '@tanstack/react-table'; export type DataTableArrowPaginatedProps = { className?: string; table: arrow.Table | undefined; fontSize?: DataTablePaginatedProps<any>['fontSize']; footerActions?: DataTablePaginatedProps<any>['footerActions']; pageSize?: number; /** Optional custom value formatter for binary/geometry data */ formatValue?: ArrowDataTableValueFormatter; + /** Enable checkbox-based row selection */ + enableRowSelection?: boolean; + /** Controlled row selection state */ + rowSelection?: RowSelectionState; + /** Callback fired when selection changes */ + onRowSelectionChange?: (selection: RowSelectionState) => void; }; export const DataTableArrowPaginated: FC<DataTableArrowPaginatedProps> = ({ className, table, fontSize, footerActions, pageSize = 100, formatValue, + enableRowSelection, + rowSelection, + onRowSelectionChange, }) => { // ... existing pagination logic ... return ( <DataTablePaginated className={className} data={adt.data} columns={adt.columns} numRows={table?.numRows} pagination={pagination} onPaginationChange={setPagination} fontSize={fontSize} footerActions={footerActions} + enableRowSelection={enableRowSelection} + rowSelection={rowSelection} + onRowSelectionChange={onRowSelectionChange} /> ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/data-table/src/DataTableArrowPaginated.tsx` around lines 11 - 19, DataTableArrowPaginated currently doesn't accept or forward selection props, preventing consumers from using enableRowSelection/rowSelection/onRowSelectionChange when working with arrow.Table; update the DataTableArrowPaginatedProps type to include enableRowSelection?: boolean, rowSelection?: whatever shape DataTablePaginated expects, and onRowSelectionChange?: (args) => void (matching DataTablePaginated's signatures), then pass these props through from the DataTableArrowPaginated component into the rendered DataTablePaginated (search for the DataTableArrowPaginated function/component and the JSX where it renders DataTablePaginated) so Arrow-table consumers can control selection without dropping to the lower-level API.packages/sql-editor/src/components/QueryResultPanel.tsx (1)
16-16: Consider importingRowSelectionStatefrom@sqlrooms/data-tablefor consistency.Since
@sqlrooms/data-tablealready re-exportsRowSelectionState(perindex.tsline 37), and this file imports other symbols from that package (lines 2–5), importing it from the same barrel would keep the dependency surface tighter.-import type {Row, RowSelectionState} from '@tanstack/react-table'; +import type {Row} from '@tanstack/react-table'; +import type {RowSelectionState} from '@sqlrooms/data-table';Not critical since
Rowis already imported directly from@tanstack/react-table.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sql-editor/src/components/QueryResultPanel.tsx` at line 16, The import of RowSelectionState should come from the project's data-table barrel for consistency: update the import statements in QueryResultPanel.tsx to import RowSelectionState from '@sqlrooms/data-table' instead of '@tanstack/react-table' (keep Row imported from '@tanstack/react-table' as-is); adjust the existing import group that already pulls symbols from '@sqlrooms/data-table' to include RowSelectionState so all shared types are re-exported from the same module.packages/data-table/src/DataTablePaginated.tsx (1)
182-202: Header layout and sticky positioning looks correct.The
top-[-1px]on the row-number/select-all header andtop-0on the trailing header are slightly inconsistent — verify this doesn't cause a 1px visual gap in the sticky header row. If intentional (to hide the border overlap), a brief comment would help future readers.Also applies to: 246-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/data-table/src/DataTablePaginated.tsx` around lines 182 - 202, The sticky header uses inconsistent top offsets (TableHead with class `top-[-1px]` for the row-number/select-all cell vs `top-0` on the trailing header), which can cause a 1px gap; either normalize both to the same top offset (e.g., change `top-[-1px]` to `top-0` or vice versa) so all sticky cells align, or keep the -1px intentional and add a short inline comment next to the TableHead className and the corresponding trailing header instance explaining that the -1px is used to hide border overlap; update both occurrences (the header at the shown block and the other at the second occurrence around line 246) so behavior is consistent and documented.
🤖 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/whats-new.md`:
- Around line 25-27: Update the example imports to avoid depending on TanStack
internals: replace the import of RowSelectionState from '@tanstack/table-core'
with an import from '@sqlrooms/data-table' (and keep React's useState import
as-is). Specifically, change the module that exports RowSelectionState in the
example so it comes from '@sqlrooms/data-table' to prevent a transitive
dependency on `@tanstack/table-core` while preserving the symbol RowSelectionState
and the useState usage.
In `@packages/data-table/src/DataTablePaginated.tsx`:
- Around line 145-151: The onRowSelectionChange handler contains a redundant
if/else that calls handleRowSelectionChange(update) in both branches; simplify
by removing the conditional and directly calling
handleRowSelectionChange(update) in the onRowSelectionChange property (refer to
the onRowSelectionChange prop and the handleRowSelectionChange function to
locate the code).
- Around line 114-131: handleRowSelectionChange currently resolves functional
updaters using the closed-over currentRowSelection which causes lost updates in
uncontrolled mode; change the logic so that if onRowSelectionChange is provided
you resolve the updater to a concrete newValue and call
onRowSelectionChange(newValue), but if uncontrolled you pass the updater
directly into setInternalRowSelection (i.e. forward the function instead of
resolving it against currentRowSelection) so React can apply queued updates
correctly; update the dependency array to remove currentRowSelection and only
include onRowSelectionChange and setInternalRowSelection to avoid unnecessary
callback churn.
---
Nitpick comments:
In `@packages/data-table/src/DataTableArrowPaginated.tsx`:
- Around line 11-19: DataTableArrowPaginated currently doesn't accept or forward
selection props, preventing consumers from using
enableRowSelection/rowSelection/onRowSelectionChange when working with
arrow.Table; update the DataTableArrowPaginatedProps type to include
enableRowSelection?: boolean, rowSelection?: whatever shape DataTablePaginated
expects, and onRowSelectionChange?: (args) => void (matching
DataTablePaginated's signatures), then pass these props through from the
DataTableArrowPaginated component into the rendered DataTablePaginated (search
for the DataTableArrowPaginated function/component and the JSX where it renders
DataTablePaginated) so Arrow-table consumers can control selection without
dropping to the lower-level API.
In `@packages/data-table/src/DataTablePaginated.tsx`:
- Around line 182-202: The sticky header uses inconsistent top offsets
(TableHead with class `top-[-1px]` for the row-number/select-all cell vs `top-0`
on the trailing header), which can cause a 1px gap; either normalize both to the
same top offset (e.g., change `top-[-1px]` to `top-0` or vice versa) so all
sticky cells align, or keep the -1px intentional and add a short inline comment
next to the TableHead className and the corresponding trailing header instance
explaining that the -1px is used to hide border overlap; update both occurrences
(the header at the shown block and the other at the second occurrence around
line 246) so behavior is consistent and documented.
In `@packages/sql-editor/src/components/QueryResultPanel.tsx`:
- Line 16: The import of RowSelectionState should come from the project's
data-table barrel for consistency: update the import statements in
QueryResultPanel.tsx to import RowSelectionState from '@sqlrooms/data-table'
instead of '@tanstack/react-table' (keep Row imported from
'@tanstack/react-table' as-is); adjust the existing import group that already
pulls symbols from '@sqlrooms/data-table' to include RowSelectionState so all
shared types are re-exported from the same module.
Signed-off-by: Ilya Boyandin <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/data-table/src/DataTablePaginated.tsx`:
- Line 248: The trailing sticky header uses top-0 which creates a 1px seam vs
other header cells using top-[-1px]; update the TableHead instance in
DataTablePaginated (the element: TableHead className="bg-background sticky top-0
w-full whitespace-nowrap border-r border-t py-2") to use the same top-[-1px]
offset so the sticky header aligns exactly with its siblings and removes the
hairline gap while keeping the rest of the classes unchanged.
- Around line 109-113: If a caller supplies rowSelection but not
onRowSelectionChange the component ends up visually frozen because
currentRowSelection is always the prop; change the logic so when rowSelection
!== undefined && onRowSelectionChange === undefined you fall back to
uncontrolled behavior and use internalRowSelection for currentRowSelection (and
emit a dev-only console.warn to surface the mismatch). Update the lines
computing currentRowSelection to detect this partial-controlled case
(referencing rowSelection, onRowSelectionChange, internalRowSelection,
setInternalRowSelection and currentRowSelection) and ensure any checkbox inputs
are driven by internalRowSelection in that branch so UI interactions update
visually.
---
Duplicate comments:
In `@packages/data-table/src/DataTablePaginated.tsx`:
- Around line 151-153: Replace the unnecessary arrow-function wrapper used for
the onRowSelectionChange prop with the handler function itself: currently
onRowSelectionChange: (update) => { handleRowSelectionChange(update); } should
be simplified to pass handleRowSelectionChange directly so the prop reads
onRowSelectionChange: handleRowSelectionChange (preserving the same signature
and avoiding creating a new function identity each render).
Comment on lines
+109
to
+113
| const [internalRowSelection, setInternalRowSelection] = | ||
| useState<RowSelectionState>({}); | ||
|
|
||
| // Use controlled or uncontrolled row selection | ||
| const currentRowSelection = rowSelection ?? internalRowSelection; |
Contributor
There was a problem hiding this comment.
Partially-controlled mode leaves checkboxes visually unresponsive.
When rowSelection is passed but onRowSelectionChange is omitted, currentRowSelection is always the rowSelection prop (never undefined). Any checkbox interaction flows to setInternalRowSelection, updating internalRowSelection, but the ?? fallback is never reached — the displayed state tracks the prop, not the internal state. Checkboxes will appear interactive but never visually respond.
Either add a console.warn in dev mode when rowSelection is provided without onRowSelectionChange, or explicitly disable the checkboxes in that case (treating it as a read-only display), or document the invariant clearly in the prop JSDoc.
💡 Lightweight dev guard
+ useEffect(() => {
+ if (
+ process.env.NODE_ENV !== 'production' &&
+ rowSelection !== undefined &&
+ !onRowSelectionChange
+ ) {
+ console.warn(
+ 'DataTablePaginated: `rowSelection` was provided without `onRowSelectionChange`. ' +
+ 'Row selection will not be interactive. Provide `onRowSelectionChange` to enable controlled selection.',
+ );
+ }
+ }, [rowSelection, onRowSelectionChange]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/data-table/src/DataTablePaginated.tsx` around lines 109 - 113, If a
caller supplies rowSelection but not onRowSelectionChange the component ends up
visually frozen because currentRowSelection is always the prop; change the logic
so when rowSelection !== undefined && onRowSelectionChange === undefined you
fall back to uncontrolled behavior and use internalRowSelection for
currentRowSelection (and emit a dev-only console.warn to surface the mismatch).
Update the lines computing currentRowSelection to detect this partial-controlled
case (referencing rowSelection, onRowSelectionChange, internalRowSelection,
setInternalRowSelection and currentRowSelection) and ensure any checkbox inputs
are driven by internalRowSelection in that branch so UI interactions update
visually.
Signed-off-by: Ilya Boyandin <[email protected]>
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
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.
Summary by CodeRabbit
New Features
Style
Documentation