fix(data-grid): persist cell updates when filtering is applied#1116
Merged
fix(data-grid): persist cell updates when filtering is applied#1116
Conversation
Fixes issue where cell edits in data-grid-live were not persisted to the database when filters were active. The problem was that onDataUpdate was building a newData array with only the filtered row count instead of the complete dataset. When filters are applied: - The table's row model contains only filtered rows - But the data prop contains all rows (unfiltered) - onDataUpdate needs to return all rows with updates applied This fix ensures that when building the updated data array, we iterate over the complete currentData array (all rows) rather than just the visible filtered rows (tableRowCount). This allows onDataChange to properly detect and persist all updates to the database. Fixes #1106 Co-authored-by: Cursor <[email protected]>
Added comprehensive tests to verify that cell updates work correctly when column filters are active: 1. Single filtered row update: Verifies that updating a cell in a filtered view correctly updates the full dataset (all 3 rows) rather than just the filtered rows 2. Multiple filtered rows update: Tests updating a cell when multiple rows are visible through filtering, ensuring the correct row in the full dataset is updated These tests ensure the fix for #1106 is working correctly and prevent regression of the filtering + cell update bug. All 96 tests passing. Co-authored-by: Cursor <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a persistence bug in useDataGrid where cell edits made while column filters are active were not being applied back onto the full underlying dataset, causing DB updates to miss unfiltered rows.
Changes:
- Update
onDataUpdateto always rebuildnewDatausingcurrentData.length(full dataset), not the filtered row count. - Add tests ensuring edits while filtered are applied to the correct underlying row and
onDataChangereceives the full dataset. - Update registry JSON snapshots to reflect
onRowSelectusingrow.id(consistent with current source/types).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/hooks/use-data-grid.ts |
Fixes filtered-edit persistence by applying updates onto the full data array. |
src/hooks/test/use-data-grid.test.tsx |
Adds filter-aware edit tests and a simple filter function for the test columns. |
public/r/data-grid-select-column.json |
Registry snapshot updated to use row.id for selection callbacks. |
public/r/data-grid-filter-menu.json |
Registry snapshot updated to reflect onRowSelect signature using rowId. |
Comments suppressed due to low confidence (3)
src/hooks/use-data-grid.ts:411
if (!existingRow)treats valid falsy row values (e.g.0,false,"") as missing and replaces them with{}. SinceTDatais unconstrained, prefer a nullish check (existingRow == null) and avoid synthesizing an empty object unless the data model explicitly guarantees object rows.
if (!existingRow) {
newData[i] = {} as TData;
continue;
}
src/hooks/use-data-grid.ts:406
onDataUpdaterebuildsnewDataby looping overcurrentData.lengthon every edit. With large datasets (and especially during rapid edits/paste), this becomes O(n) per update even when only a few rows changed. Consider starting from a shallow copy ofcurrentDataand only cloning/applying changes for the indices present inrowUpdatesMap.
const newData: TData[] = new Array(currentData.length);
for (let i = 0; i < currentData.length; i++) {
const updates = rowUpdatesMap.get(i);
const existingRow = currentData[i];
src/hooks/test/use-data-grid.test.tsx:40
- In
simpleFilterFn, the parameter is named_columnIdbut is used. Rename it tocolumnId(or similar) to avoid the conventional meaning of a leading underscore (intentionally unused).
const simpleFilterFn = (
row: { getValue: (id: string) => unknown },
_columnId: string,
filterValue: string,
) => {
const value = String(row.getValue(_columnId) ?? "");
return value.toLowerCase().includes(filterValue.toLowerCase());
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Modified
onDataUpdateinuse-data-grid.tsto iterate over the complete dataset (currentData.length) instead of just filtered rows (tableRowCount). When filters were active, the function was building a partial data array containing only visible rows, causingonDataChangeto miss updates for the complete dataset and preventing persistence to the database.Fixes #1106