Open
Conversation
github-actions
bot
added
react
laurafg
force-pushed
the
feature/editable-table-selector
branch
from
c5b9588 to
4b47bd1
Compare
Contributor
📦 Alpha Package Version PublishedUse Use |
Contributor
🔍 Visual review for your branch is published 🔍Here are the links to: |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to fix selection behavior for nested rows in the experimental OneDataCollection Table by ensuring nested child records can be selected even when they aren’t part of the top-level data.records for the current page.
Changes:
- Extend
useSelectableto persist an item reference when selecting by record object (to support nested children not found indata.records). - Update Table row components to pass the full record into the selection handler (
onSelectItem(item, checked)), enabling the hook to store references for nested rows. - Adjust Storybook mock data to ensure nested records have unique IDs and update a story selectable function accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/hooks/datasource/useSelectable/useSelectable.ts | Attempts to include nested-row items in page-only selection by using resolved item refs; adds optional itemRef to internal selection updates. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/RowLoading/index.tsx | Renames prop wiring from onCheckedChange to onSelectItem for consistency with new API. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/Row.tsx | Changes selection callback to pass (item, checked) so nested rows can provide a record reference. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/NestedRow.tsx | Updates nested row prop types to the new onSelectItem(item, checked) signature. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/LoadMore/index.tsx | Overrides source.selectable for the Load More row (intended to suppress checkbox rendering). |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/Table.tsx | Passes the new onSelectItem handler through to Row/MotionRow. |
| packages/react/src/experimental/OneDataCollection/stories/visualizations/editable-table/editable-table.stories.tsx | Updates selectable in a story to return item.id (valid selection id). |
| packages/react/src/experimental/OneDataCollection/stories/mockData.tsx | Ensures nested mock records have unique IDs to avoid selection collisions. |
Comment on lines
+236
to
+242
| // In page-only mode, include items from the current page OR items | ||
| // with a resolved item reference (e.g. children loaded via nested rows | ||
| // that aren't in data.records but are visible on the current page). | ||
| if (isPageOnlySelection && currentPageItemIds) { | ||
| return currentPageItemIds.has(itemState.id) | ||
| return ( | ||
| currentPageItemIds.has(itemState.id) || itemState.item !== undefined | ||
| ) |
Comment on lines
251
to
253
| if (isPageOnlySelection && currentPageItemIds) { | ||
| return currentPageItemIds.has(id) | ||
| return currentPageItemIds.has(id) || itemState.item !== undefined | ||
| } |
| return ( | ||
| <Row | ||
| {...props} | ||
| source={{ ...props.source, selectable: () => undefined }} |
Comment on lines
+236
to
+242
| // In page-only mode, include items from the current page OR items | ||
| // with a resolved item reference (e.g. children loaded via nested rows | ||
| // that aren't in data.records but are visible on the current page). | ||
| if (isPageOnlySelection && currentPageItemIds) { | ||
| return currentPageItemIds.has(itemState.id) | ||
| return ( | ||
| currentPageItemIds.has(itemState.id) || itemState.item !== undefined | ||
| ) |
Contributor
Contributor
📱 Expo Go Preview Published
The onCheckedChange closure in Table.tsx captured the parent item and
Made-with: Cursor`
LinksQR Code |
laurafg
force-pushed
the
feature/editable-table-selector
branch
from
4b47bd1 to
d1b113c
Compare
The onCheckedChange closure in Table.tsx captured the parent item and was inherited by child rows via prop spreading in NestedRow, so clicking a child checkbox selected the parent instead. Additionally, useSelectable discarded the item reference for children (not in data.records) and filtered them out of itemsStatus/selectedIds via currentPageItemIds. - Replace onCheckedChange with onSelectItem prop so each Row calls handleSelectItemChange with its own item - Pass item reference through handleSelectItemChangeInternal so children are stored with their actual item object - Allow items with resolved references through the page-only selection filter (children are on the current page but not in data.records) - Hide checkbox on LoadMore rows Made-with: Cursor
When "select all" is clicked, only top-level parents were selected because useSelectable iterates data.records which doesn't include lazily-loaded children. Added a useEffect in NestedRow that detects parent selection transitions and cascades to loaded children. Made-with: Cursor
laurafg
force-pushed
the
fix/nested-rows-selectable
branch
from
9339a45 to
70b5306
Compare
Contributor
✅ No New Circular DependenciesNo new circular dependencies detected. Current count: 0 |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes broken selection behavior for nested rows in the experimental OneDataCollection Table, including child-row selection, select-all cascading, parent indeterminate state, and Storybook data issues.
Changes:
- Updates Table row components to use an
onSelectItem(item, checked)API so each row selects itself (and can cascade to children). - Extends nested-row selection logic to compute indeterminate/checked state from children and cascade select-all to loaded children.
- Adjusts
useSelectableto better support nested items and fixes Storybook mock data to avoid duplicate IDs.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/hooks/datasource/useSelectable/useSelectable.ts | Adds item reference support during selection and adjusts page-only filtering/select-all behavior. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/RowLoading/index.tsx | Renames row selection prop to onSelectItem. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/Row.tsx | Switches selection callback signature and supports indeterminate/children-selected checkbox rendering. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/NestedRow.tsx | Implements child→parent indeterminate/checked logic and select-all cascading to loaded children. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/LoadMore/index.tsx | Disables checkbox rendering on “Load more” rows while preserving column alignment. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/Table.tsx | Passes handleSelectItemChange directly via onSelectItem. |
| packages/react/src/experimental/OneDataCollection/stories/visualizations/table/table.stories.tsx | Fixes selectable to return item.id for nested records story. |
| packages/react/src/experimental/OneDataCollection/stories/visualizations/editable-table/editable-table.stories.tsx | Fixes selectable to return item.id for nested records story. |
| packages/react/src/experimental/OneDataCollection/stories/mockData.tsx | Ensures nested mock records have unique IDs to avoid selection collisions. |
Comments suppressed due to low confidence (1)
packages/react/src/hooks/datasource/useSelectable/useSelectable.ts:581
handleSelectAllno longer readsallSelectedCheck, but it’s still listed in theuseCallbackdependency array. This creates unnecessary callback churn and makes the dependencies misleading—removeallSelectedCheck(or reintroduce a use if it’s still intended).
[
isMultiSelection,
allSelectedCheck,
isGrouped,
data,
Comment on lines
235
to
+242
| if (itemState.item === undefined) return false | ||
| // Filter to only current page items in page-only selection mode | ||
| // In page-only mode, include items from the current page OR items | ||
| // with a resolved item reference (e.g. children loaded via nested rows | ||
| // that aren't in data.records but are visible on the current page). | ||
| if (isPageOnlySelection && currentPageItemIds) { | ||
| return currentPageItemIds.has(itemState.id) | ||
| return ( | ||
| currentPageItemIds.has(itemState.id) || itemState.item !== undefined | ||
| ) |
Comment on lines
248
to
253
| const selectedIds = Array.from(items.entries()) | ||
| .filter(([id, itemState]) => { | ||
| if (!itemState.checked) return false | ||
| // Filter to only current page items in page-only selection mode | ||
| if (isPageOnlySelection && currentPageItemIds) { | ||
| return currentPageItemIds.has(id) | ||
| return currentPageItemIds.has(id) || itemState.item !== undefined | ||
| } |
Comment on lines
422
to
+426
| const handleSelectItemChangeInternal = useCallback( | ||
| ( | ||
| itemId: SelectionId | readonly SelectionId[], | ||
| checked: boolean, | ||
| onlyIfNotPreviousState: boolean = false | ||
| onlyIfNotPreviousState: boolean = false, |
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.
🚪 Why?
Selecting children rows in a DataCollection with nested records was broken in multiple ways: clicking a child checkbox selected the parent instead, children were excluded from bulk actions, "select all" only selected top-level parents (ignoring nested children), and Storybook stories had duplicate IDs causing all items to be checked at once.
🔑 What?
Fix children row selection:
onCheckedChangewithonSelectItemprop inRow/NestedRowso each row callshandleSelectItemChangewith its own item instead of inheriting the parent's closureuseSelectableto accept and store the item reference directly inhandleSelectItemChangeInternal, preventing failed lookups for children not present indata.recordscurrentPageItemIdsfilter initemsStatus/selectedIdsto include items with a resolved reference (children are visible on the current page but not in top-level records)Cascade "select all" to nested children:
useEffectinNestedRowthat detects when a parent transitions from unselected → selected (e.g., via "select all") and cascades selection to all loaded childrenhandleSelectItemWithChildrenclick handlerParent indeterminate/checked state from children:
parentIndeterminateandallChildrenSelectedinNestedRowbased on loaded children's selection stateuseEffectRowfor correct checkbox renderingHide checkbox on "load more" rows:
selectableto() => undefinedonLoadMorerows so the column remains rendered (preserving table alignment) but no checkbox is displayedFix Storybook mock data:
mockData.tsx(${user.id}-child-0, etc.)selectableprop in nested records stories to useitem.idinstead of a hardcoded empty stringEdge cases handled
useSelectable, then theuseEffectinNestedRowcascades to all loaded childrenhandleSelectAll(false)resets the entire items Map to empty, clearing both parents and children in one operationhandleSelectItemWithChildrencascades immediately on click; theuseEffectdoes not double-trigger because the ref already tracks the parent as selectedchildrendependency changes, theuseEffectre-runs, sees the parent is selected, and cascades to newly loaded childrenuseEffectdoes not re-select the child because the ref already haswasSelected = true(no transition detected)allChildrenSelectedeffect (which detects 0 selected children and does nothing), and the indeterminate logic resetsKnown limitation: selection scope is tied to expanded rows
"Select all" can only cascade to children that have been loaded — i.e., the parent row was previously expanded. Children of collapsed/never-expanded rows are not selected because they don't exist in memory yet (they are lazily loaded via
NestedDataProvider).This means both selection and count only reflect what has been expanded:
useEffectfires (parent is already selected → new children loaded → auto-selected), and the count increases.Counter accuracy per mode:
checkedCount(actual checked items in state). It accurately reflects what is selected — parents + expanded children. As you expand more rows, children get cascaded and the count goes up incrementally.selectAllTotal - uncheckedCount, whereselectAllTotalcomes frompaginationInfo.total(top-level items only). This formula does not account for children at all, so even after children are loaded and selected, the displayed count can be lower than the actual number of selected items. This is a pre-existing architectural limitation of howuseSelectabletracks counts separately fromNestedDataProvider.