Repository navigation
Conversation
Code Metrics Report
Details | | main (331e50f) | #604 (90be8d0) | +/- |
|---------------------|----------------|----------------|---------|
+ | Coverage | 88.6% | 88.7% | +0.0% |
| Files | 208 | 208 | 0 |
| Lines | 6263 | 6293 | +30 |
+ | Covered | 5552 | 5582 | +30 |
+ | Test Execution Time | 16m34s | 1m38s | -14m56s |Code coverage of files in pull request scope (98.9% → 99.1%, patch 100.0%)
Reported by octocov |
|
/review |
|
Code review complete for PR #604: no High/Medium issues found in changed package code, and this workflow does not expose a general PR comment tool for an approval note.
|
There was a problem hiding this comment.
🟢 Approval recommended
The public contract, implementation, compatibility behavior, exports, tests, and documentation are aligned.
0 open findings
What changed in this PR
Adds controlled and initialized-uncontrolled row selection to DataTable while preserving legacy callback compatibility and cross-page selection actions.
Changes:
- Introduces and exports
RowSelectionOptions. - Adds controlled selection and row-history handling.
- Adds comprehensive tests, documentation, and a minor changeset.
| File | Description |
|---|---|
packages/core/src/index.ts |
Exports the new public type. |
packages/core/src/components/data-table/use-data-table.ts |
Implements selection ownership and row history. |
packages/core/src/components/data-table/types.ts |
Defines and documents the new API. |
packages/core/src/components/data-table/row-selection.test.tsx |
Covers controlled and uncontrolled behavior. |
packages/core/src/components/data-table/index.ts |
Adds the component-level export. |
docs/components/data-table.md |
Updates generated consumer documentation. |
docs-src/components/data-table.docs.outline.md |
Adds authored selection documentation. |
docs-manifest.json |
Updates generated documentation metadata. |
.changeset/clear-kites-select.md |
Records the minor API addition. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| | `rowActions` | `RowAction<TRow>[]` | Per-row action items rendered in a kebab-menu column. The column is omitted when empty or not provided. | | ||
| | `onSelectionChange` | `(ids: string[]) => void` | Called with selected row IDs on change. Providing this (or `selectionActions`) enables the checkbox column. Rows must have a string or number `id`. | | ||
| | `rowSelection` | `RowSelectionOptions` | Controlled `selectedIds` + `onChange`, or uncontrolled `defaultSelectedIds` + optional `onChange`. Providing even `{}` enables checkboxes. See [Row selection](#row-selection). | | ||
| | `onSelectionChange` | `(ids: string[]) => void` | **Deprecated:** use `rowSelection.onChange`. Still supported for compatibility; enables checkboxes and is used when the new callback is absent. | |
There was a problem hiding this comment.
I was going to say let's make a ticket to remove on v2.0.0 but I guess we just sweep all "deprecated" properties at that time
interacsean
left a comment
interacsean
left a comment
There was a problem hiding this comment.
Looks good to me, but Claude picked up:
Bulk actions can quietly act on only some of the “selected” rows, then clear all of them
use-data-table.ts:460, selection-bar.tsx:36, selection-bar.tsx:93, use-data-table.ts:433
What happens: before this PR, every selected ID had a row object behind it. Now defaultSelectedIds or controlled selectedIds can include IDs whose rows were never loaded, and selectedRows leaves those out.
What the user sees: the bar still counts selectedIds.length (“5 selected”). An action button only shows “(n)” when canApply narrows the list, so a destructive “Delete” shows no count but receives just the 3 loaded rows.
Why it’s a data problem: when the action succeeds, clearSelection() wipes all 5 IDs, including the 2 that were never touched. The user reasonably thinks all 5 were deleted, and the 2 skipped IDs are gone from the selection. If none of the selected rows are loaded, every action is disabled and nothing explains why.
Why it matters here: restoring a selection from app state is the main use case the PR describes, so this will happen in normal use, not just in edge cases. The docs mention that actions only use available rows, but not the clear-everything part or the misleading count.
Fix:
In the bar, show the “(n)” count whenever the rows an action receives are fewer than selectedIds.length, not only when canApply narrows them. Or add a “k not loaded” hint.
On success, remove only the IDs the action actually received, rather than calling a full clearSelection(). At minimum, document that unloaded IDs are cleared too.
Add a test that covers an action running while some selected IDs are unloaded.
Motivation
useDataTablecurrently owns row selection internally and exposesonSelectionChangeas a notification. Consumers cannot declaratively supply the selected IDs or initialize a selection from application state. This makes it harder to coordinate table selection with other controls or restore an existing selection.Add a
rowSelectionoption supporting controlledselectedIds+onChange, or uncontrolleddefaultSelectedIdswith an optional notification. The existingonSelectionChangecallback is deprecated in favor ofrowSelection.onChange, but remains supported for compatibility. Selection-action behavior is preserved.This isolates the row-selection API addition from #525 onto
main. It does not include the lint upgrade, column-persistence changes, row-expansion changes, or the broader state-ownership refactor.Design Decision
Keep the owner authoritative
The option follows the controlled/uncontrolled shape already used by row expansion. Controlled IDs require
onChangeand cannot be combined with defaults. Selection commands propose a new ID array, but rendered selection changes only when the owner updates its props. Uncontrolled defaults are applied once at mount.The current uncontrolled state and eager command updates are retained rather than introducing the larger branch's generic external store. Multiple uncontrolled commands still compose within one commit; controlled commands before an owner commit share the same base. Keep the mode stable while mounted.
Keep row history separate from selection proposals
Bulk actions need row objects, not just IDs. Remember loaded selected rows across page changes, including initial/external selections and refetched versions. Record newly selected event rows before notifying an owner that may also change page in the same commit. A rejected controlled clear or deselect request must not discard rows that remain selected.
History bookkeeping does not update React state or notify selection callbacks. Unloaded IDs remain selected IDs but are omitted from
selectedRowsuntil loaded; bulk actions receive only available records. Async action completion consults the latest committed selection and callback before requesting a clear.Preserve existing callers
onSelectionChangeis marked@deprecatedbut remains supported. MigrateonSelectionChange: handleSelectionChangetorowSelection: { onChange: handleSelectionChange }to retain uncontrolled notification behavior. When both callbacks are provided,rowSelection.onChangetakes precedence; otherwise the legacy callback remains the fallback. Header selection stays page-scoped, Clear spans all pages, and callbacks do not fire on mount or external prop updates.Summary
RowSelectionOptions, supporting controlled selection, initial uncontrolled selection, and enabling checkboxes without a dummy callback; deprecate the legacy notification prop without removing it.