fix: DataTable sorting, filtering, resizing broken with React Compiler#29
Merged
Conversation
- Add missing .SortButton CSS class that caused unstyled header buttons - Return new-identity table object from useDataTable via useMemo spread, so React Compiler detects state changes in consumer components - Fix enableFilters to include globalFilter, preventing columns from being unfilterable when only globalFilter feature is enabled
📝 WalkthroughWalkthroughAdds a comprehensive bug-focused test file for DataTable, adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/base-ui/src/components/data-table/DataTable.bugs.test.tsx`:
- Around line 319-329: The test currently enables both features.filtering and
features.globalFilter so it doesn't exercise the regression where global
filtering should work when filtering is disabled; update the test setup and
render to set features.filtering: false and features.globalFilter: true (adjust
the useDataTable call and the DataTable.Root props where features is passed) so
the hook paths in useDataTable (and any enableFilters logic) are exercised with
filtering disabled but globalFilter enabled; ensure any expectations/assertions
remain valid for this configuration.
In `@packages/base-ui/src/components/data-table/DataTable.module.css`:
- Around line 210-220: The .SortButton rule makes the header button a full-width
flex container which forces child alignment to flex-start and breaks
centered/right column alignment; update the .SortButton selector to use display:
inline-flex (not full-width flex), remove or stop forcing width: 100% and
height: 100%, and add justify-content: inherit so the button inherits the
header's alignment; this preserves numeric/center/right alignment while keeping
the button layout and gap styles.
In `@packages/base-ui/src/components/data-table/hooks/useColumnSizeVars.ts`:
- Line 1: Move the module-level directive "'use no memo';" into the start of the
useColumnSizeVars hook body so the compiler opt-out is scoped only to that
custom hook; locate the useColumnSizeVars function in this file and place the
directive as the first statement inside its function body (remove the
module-level occurrence) ensuring no other exports in the module are affected.
In `@packages/base-ui/src/components/data-table/hooks/useDataTable.ts`:
- Line 3: Remove the useMemo import/usages in useDataTable and always return a
fresh spread of the table object so consumers get a new identity each render:
stop memoizing the returned object (remove any useMemo wrapping around the
table), ensure the hook still calls useReactTable as before (look for the
`useDataTable` function and the `useReactTable` call) and return `{ ...table }`
directly; also remove `useMemo` from the import list to keep imports tidy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36850447-5479-4b5c-a65a-083b0d53d21c
📒 Files selected for processing (4)
packages/base-ui/src/components/data-table/DataTable.bugs.test.tsxpackages/base-ui/src/components/data-table/DataTable.module.csspackages/base-ui/src/components/data-table/hooks/useColumnSizeVars.tspackages/base-ui/src/components/data-table/hooks/useDataTable.ts
- Test globalFilter with filtering:false to exercise the actual regression - SortButton: use inline-flex + justify-content:inherit for alignment - Scope 'use no memo' to useColumnSizeVars function body - Drop useMemo wrapper; return plain spread for simplicity
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 `@packages/base-ui/src/components/data-table/DataTable.bugs.test.tsx`:
- Around line 222-235: The test "resize handle has mouseDown handler that starts
resizing" leaves a started resize active and may pollute other tests; after
firing fireEvent.mouseDown(firstHandle, { clientX: 100 }) add a matching
fireEvent.mouseUp(...) to end the resize (preferably on document or the same
element) so any global mouse listeners are removed; update the test around
ResizableTable / firstHandle to call mouseUp as cleanup to ensure
data-ov-resizing resets and no global listeners remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7317b33-6fe5-48ba-b90c-347a1bd2e68c
📒 Files selected for processing (4)
packages/base-ui/src/components/data-table/DataTable.bugs.test.tsxpackages/base-ui/src/components/data-table/DataTable.module.csspackages/base-ui/src/components/data-table/hooks/useColumnSizeVars.tspackages/base-ui/src/components/data-table/hooks/useDataTable.ts
Comment on lines
+222
to
+235
| it('resize handle has mouseDown handler that starts resizing', () => { | ||
| renderWithTheme(<ResizableTable />); | ||
| const resizeHandles = screen.getAllByRole('separator'); | ||
| const firstHandle = resizeHandles[0]!; | ||
|
|
||
| // Initially not resizing | ||
| expect(firstHandle).toHaveAttribute('data-ov-resizing', 'false'); | ||
|
|
||
| // Fire mouseDown to start resizing | ||
| fireEvent.mouseDown(firstHandle, { clientX: 100 }); | ||
|
|
||
| // After mouseDown, should be in resizing state | ||
| expect(firstHandle).toHaveAttribute('data-ov-resizing', 'true'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a mouseUp cleanup to avoid test pollution.
The test fires mouseDown to start resizing but doesn't fire mouseUp to end it. While this may not affect the current test in isolation, it could leave global event listeners attached if the component registers them on mouseDown.
🧪 Suggested improvement
// Fire mouseDown to start resizing
fireEvent.mouseDown(firstHandle, { clientX: 100 });
// After mouseDown, should be in resizing state
expect(firstHandle).toHaveAttribute('data-ov-resizing', 'true');
+
+ // Cleanup: end the resize operation
+ fireEvent.mouseUp(firstHandle);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/data-table/DataTable.bugs.test.tsx` around
lines 222 - 235, The test "resize handle has mouseDown handler that starts
resizing" leaves a started resize active and may pollute other tests; after
firing fireEvent.mouseDown(firstHandle, { clientX: 100 }) add a matching
fireEvent.mouseUp(...) to end the resize (preferably on document or the same
element) so any global mouse listeners are removed; update the test around
ResizableTable / firstHandle to call mouseUp as cleanup to ensure
data-ov-resizing resets and no global listeners remain.
Merging this PR will degrade performance by 86.44%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | mount section |
393.4 µs | 1,897.3 µs | -79.27% |
| ❌ | WallTime | mount 1000 |
98.1 ms | 441.1 ms | -77.76% |
| ❌ | WallTime | mount div |
434.9 µs | 2,008.2 µs | -78.35% |
| ❌ | WallTime | mount 1000 |
35.3 ms | 192.3 ms | -81.63% |
| ❌ | WallTime | variant change |
213.5 µs | 1,574.7 µs | -86.44% |
| ❌ | WallTime | mount |
560.7 µs | 2,665.6 µs | -78.97% |
| ❌ | WallTime | mount 100 rows |
41.7 ms | 218.4 ms | -80.92% |
| ❌ | WallTime | mount 1000 rows |
436.4 ms | 2,036.5 ms | -78.57% |
| ❌ | WallTime | mount 1000 |
810.3 ms | 1,953.9 ms | -58.53% |
| ❌ | WallTime | checked toggle |
1.2 ms | 5 ms | -75.27% |
| ❌ | WallTime | mount |
1.8 ms | 5.7 ms | -68.13% |
| ❌ | WallTime | mount with decorators |
614.5 µs | 3,192.1 µs | -80.75% |
Comparing feat/table-fixes (4562b52) with main (ea3c565)1
Footnotes
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.
Summary
.SortButtonCSS class caused header buttons to render as unstyled HTML, producing the ugly generic button appearanceuseReactTable()returns a stable reference that mutates internally — Compiler memoized consumer JSX based on this identity, preventing child re-renders on state changes. Fixed by returning a new-identity spread object fromuseDataTableviauseMemowith all managed state as dependenciesenableFilterswas set tofalsewhen onlyglobalFilterwas enabled (notfiltering), causing TanStack Table'scolumn.getCanGlobalFilter()to returnfalsefor all columnsTest plan
Summary by CodeRabbit
New Features
Style
Bug Fixes
Tests