refactor: forward refs in Table components#1460
Conversation
|
Warning Rate limit exceeded@kotAPI has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRefactors the Table UI component suite to use React.forwardRef across Table and all fragment components, adds typed props aliases, exposes static fragment members on the main Table export, and introduces a default export. Adds a test verifying ref forwarding to underlying DOM elements. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Consumer Code
participant T as Table (forwardRef)
participant FR as Fragments (Root/Head/Body/Row/Cell/ColumnCellHeader)
participant DOM as DOM Elements
Dev->>T: import Table (with static fragments)
Dev->>FR: <Table.Root ref=rootRef>...</Table.Root>
note over FR,DOM: Each fragment is a forwardRef wrapper
FR->>DOM: Render underlying element (div/thead/tbody/tr/th/td) with ref
Dev->>T: <Table ref=tableRef /> (if used)
T->>Dev: Warns and returns null (runtime unchanged)
FR-->>Dev: refs resolve to native elements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/components/ui/Table/fragments/TableCell.tsx (1)
3-3: Fix clsx import (named import will be undefined at runtime)clsx has a default export. Use default import to avoid runtime errors.
-import { clsx } from 'clsx'; +import clsx from 'clsx';src/components/ui/Table/fragments/TableBody.tsx (2)
3-3: Correct the clsx importSwitch to default import.
-import { clsx } from 'clsx'; +import clsx from 'clsx';
1-21: Replace namedclsximports with default importsNamed imports of
clsxfrom'clsx'are incorrect and will break compilation/runtime. Update all occurrences to use the default import:rg -lP "import\s*\{\s*clsx\s*\}\s*from\s*['\"]clsx['\"]" -g "src/**" \ | xargs -n1 sed -i "s/import { clsx } from 'clsx'/import clsx from 'clsx'/g"src/components/ui/Table/fragments/TableColumnCellHeader.tsx (1)
3-3: Fix clsx import (named vs default).clsx exports a default; named import risks runtime undefined.
Apply:
-import { clsx } from 'clsx'; +import clsx from 'clsx';src/components/ui/Table/fragments/TableRoot.tsx (1)
3-3: Fix clsx import (named vs default).Same issue here.
Apply:
-import { clsx } from 'clsx'; +import clsx from 'clsx';src/components/ui/Table/fragments/TableHead.tsx (1)
3-3: Fix clsx import (named vs default).Same as other fragments.
Apply:
-import { clsx } from 'clsx'; +import clsx from 'clsx';
🧹 Nitpick comments (5)
src/components/ui/Table/fragments/TableBody.tsx (1)
9-17: Minor: simplify className handling and prop orderNo need for default ''. Spread props before className for consistency.
-const TableBody = React.forwardRef<React.ElementRef<'tbody'>, TableBodyProps>( - ({ children, className = '', ...props }, ref) => { +const TableBody = React.forwardRef<React.ElementRef<'tbody'>, TableBodyProps>( + ({ children, className, ...props }, ref) => { return ( - <tbody ref={ref} className={clsx(className)} {...props}> + <tbody ref={ref} {...props} className={clsx(className)}> {children} </tbody> ); } );src/components/ui/Table/fragments/TableColumnCellHeader.tsx (1)
12-17: Prefer default scope="col" for column headers (a11y).Makes headers semantically correct and improves AT behavior; still overridable.
Apply:
->(({ children, className = 'cell-header', ...props }, ref) => { +>(({ children, className = 'cell-header', scope = 'col', ...props }, ref) => { return ( - <th ref={ref} className={clsx(className)} {...props}> + <th ref={ref} className={clsx(className)} scope={scope} {...props}> {children} </th> );src/components/ui/Table/fragments/TableRoot.tsx (1)
.
8-16: Allow passing props to the innerCurrently all props go to the wrapper, preventing aria/attrs on the table. Add tableProps passthrough.
Apply:
type TableRootProps = React.ComponentPropsWithoutRef<'div'> & { customRootClass?: string; + tableProps?: React.ComponentPropsWithoutRef<'table'>; }; -const TableRoot = React.forwardRef<React.ElementRef<'div'>, TableRootProps>(({ +const TableRoot = React.forwardRef<React.ElementRef<'div'>, TableRootProps>(({ children, className = '', customRootClass = '', + tableProps = {}, ...props }, ref) => { @@ - return <div ref={ref} className={clsx(`${rootClass}-wrapper`, className)} {...props} > + return <div ref={ref} className={clsx(`${rootClass}-wrapper`, className)} {...props} > {/* Todo: need to break this down into its own wrapper component */} - <table className={clsx(rootClass)}> + <table className={clsx(rootClass)} {...tableProps}> {children} </table> </div>;Also applies to: 24-29
src/components/ui/Table/tests/Table.test.tsx (2)
82-86: Avoid Object.values() for expected cell order (flakiness risk).Tie assertions to employeeKey to match render mapping order.
Apply:
- dataCells.forEach((cell, cellIndex) => { - expect(cell.textContent).toEqual( - Object.values(employeeData[rowIndex])[cellIndex].toString() - ); - }); + dataCells.forEach((cell, cellIndex) => { + const key = employeeKey[cellIndex].id as keyof (typeof employeeData)[number]; + expect(cell.textContent).toEqual(String(employeeData[rowIndex][key])); + });
24-47: Optional: add a smoke test for ColumnHeader alias.If we ship Table.ColumnHeader (alias), add a tiny render to assert it mounts without crashing.
Would you like me to push a test update that covers the alias?
.📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/components/ui/Table/Table.tsx(1 hunks)src/components/ui/Table/fragments/TableBody.tsx(1 hunks)src/components/ui/Table/fragments/TableCell.tsx(1 hunks)src/components/ui/Table/fragments/TableColumnCellHeader.tsx(1 hunks)src/components/ui/Table/fragments/TableHead.tsx(1 hunks)src/components/ui/Table/fragments/TableRoot.tsx(1 hunks)src/components/ui/Table/fragments/TableRow.tsx(1 hunks)src/components/ui/Table/tests/Table.test.tsx(1 hunks)🧰 Additional context used
🧬 Code graph analysis (2)
src/components/ui/Table/tests/Table.test.tsx (1)
docs/app/docs/components/table/docs/anatomy.tsx (1)
Table(3-18)src/components/ui/Table/Table.tsx (1)
docs/app/docs/components/table/docs/anatomy.tsx (1)
Table(3-18)🪛 GitHub Check: lint
src/components/ui/Table/Table.tsx
[failure] 30-30:
Component definition is missing display name🪛 GitHub Actions: Lint
src/components/ui/Table/Table.tsx
[error] 30-30: ESLint: react/display-name: Component definition is missing display name.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
src/components/ui/Table/fragments/TableCell.tsx (1)
7-7: Prop typing looks goodUsing ComponentPropsWithoutRef<'td'> is correct and future-proof.
src/components/ui/Table/fragments/TableRow.tsx (1)
7-7: Type alias is appropriateComponentPropsWithoutRef<'tr'> is the right surface here.
src/components/ui/Table/fragments/TableBody.tsx (1)
7-7: Typing is solidComponentPropsWithoutRef<'tbody'> is correct.
src/components/ui/Table/fragments/TableColumnCellHeader.tsx (1)
9-18: Ref forwarding implementation looks solid.Correct generics and ref wiring to
. src/components/ui/Table/fragments/TableRoot.tsx (1)
12-30: Ref forwarding to wrapper div is correct.Structure and prop spread look fine.
src/components/ui/Table/tests/Table.test.tsx (1)
90-121: Nice coverage for ref forwarding.Covers Root, Head, Body, Row, ColumnCellHeader, Cell.
src/components/ui/Table/fragments/TableHead.tsx (1)
9-17: Ref forwarding implementation looks good.Correct types and ref plumbing to
| const TableCell = React.forwardRef<React.ElementRef<'td'>, TableCellProps>( | ||
| ({ children, className = 'cell', ...props }, ref) => { | ||
| return ( | ||
| <td ref={ref} className={clsx(className)} {...props}> | ||
| {children} | ||
| </td> | ||
| ); | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Keep base styles when consumers pass className
Right now, passing className replaces the default "cell" styles. Merge instead so base styles always apply.
-const TableCell = React.forwardRef<React.ElementRef<'td'>, TableCellProps>(
- ({ children, className = 'cell', ...props }, ref) => {
+const TableCell = React.forwardRef<React.ElementRef<'td'>, TableCellProps>(
+ ({ children, className, ...props }, ref) => {
return (
- <td ref={ref} className={clsx(className)} {...props}>
+ <td ref={ref} {...props} className={clsx('cell', className)}>
{children}
</td>
);
}
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const TableCell = React.forwardRef<React.ElementRef<'td'>, TableCellProps>( | |
| ({ children, className = 'cell', ...props }, ref) => { | |
| return ( | |
| <td ref={ref} className={clsx(className)} {...props}> | |
| {children} | |
| </td> | |
| ); | |
| } | |
| ); | |
| const TableCell = React.forwardRef<React.ElementRef<'td'>, TableCellProps>( | |
| ({ children, className, ...props }, ref) => { | |
| return ( | |
| <td ref={ref} {...props} className={clsx('cell', className)}> | |
| {children} | |
| </td> | |
| ); | |
| } | |
| ); |
🤖 Prompt for AI Agents
In src/components/ui/Table/fragments/TableCell.tsx around lines 9 to 17, the
component currently lets a consumer-passed className replace the default "cell"
class; change the clsx usage to merge the base "cell" class with any incoming
className (e.g., clsx('cell', className)) so base styles always apply while
allowing overrides or additions; ensure the default param can be removed or left
as undefined and that props spreading and ref forwarding remain unchanged.
| @@ -3,11 +3,18 @@ import React from 'react'; | |||
| import { clsx } from 'clsx'; | |||
There was a problem hiding this comment.
Use default import for clsx
Prevents undefined import issues depending on TS/Bundler config.
-import { clsx } from 'clsx';
+import clsx from 'clsx';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { clsx } from 'clsx'; | |
| import clsx from 'clsx'; |
🤖 Prompt for AI Agents
In src/components/ui/Table/fragments/TableRow.tsx around line 3, the current
named import "import { clsx } from 'clsx';" can resolve to undefined under
certain TS/bundler configs; change it to the default import "import clsx from
'clsx';" and update any usages if necessary to use the default identifier to
ensure the function is correctly imported at runtime.
| const TableRow = React.forwardRef<React.ElementRef<'tr'>, TableRowProps>( | ||
| ({ children, className = 'row', ...props }, ref) => { | ||
| return ( | ||
| <tr ref={ref} className={clsx(className)} {...props}> | ||
| {children} | ||
| </tr> | ||
| ); | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve base "row" styles when className is provided
Merge consumer classes with the base to avoid accidental style loss.
-const TableRow = React.forwardRef<React.ElementRef<'tr'>, TableRowProps>(
- ({ children, className = 'row', ...props }, ref) => {
+const TableRow = React.forwardRef<React.ElementRef<'tr'>, TableRowProps>(
+ ({ children, className, ...props }, ref) => {
return (
- <tr ref={ref} className={clsx(className)} {...props}>
+ <tr ref={ref} {...props} className={clsx('row', className)}>
{children}
</tr>
);
}
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const TableRow = React.forwardRef<React.ElementRef<'tr'>, TableRowProps>( | |
| ({ children, className = 'row', ...props }, ref) => { | |
| return ( | |
| <tr ref={ref} className={clsx(className)} {...props}> | |
| {children} | |
| </tr> | |
| ); | |
| } | |
| ); | |
| const TableRow = React.forwardRef<React.ElementRef<'tr'>, TableRowProps>( | |
| ({ children, className, ...props }, ref) => { | |
| return ( | |
| <tr ref={ref} {...props} className={clsx('row', className)}> | |
| {children} | |
| </tr> | |
| ); | |
| } | |
| ); |
🤖 Prompt for AI Agents
In src/components/ui/Table/fragments/TableRow.tsx around lines 9 to 17, the
component currently replaces the base "row" class when a consumer provides
className; change the className resolution to always include the base "row" and
merge consumer classes (e.g., use clsx('row', className) or equivalent) so
consumer styles are added without losing the base styles.
| interface TableComponent | ||
| extends React.ForwardRefExoticComponent< | ||
| TableProps & React.RefAttributes<HTMLTableElement> | ||
| > { | ||
| Root: typeof TableRoot; | ||
| Body: typeof TableBody; | ||
| Head: typeof TableHead; | ||
| Row: typeof TableRow; | ||
| Cell: typeof TableCell; | ||
| ColumnCellHeader: typeof TableColumnCellHeader; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Provide ColumnHeader alias to match docs and avoid breaking consumers.
Docs show <Table.ColumnHeader/>, code exposes ColumnCellHeader only. Add a typed alias.
Apply:
interface TableComponent
extends React.ForwardRefExoticComponent<
TableProps & React.RefAttributes<HTMLTableElement>
> {
Root: typeof TableRoot;
Body: typeof TableBody;
Head: typeof TableHead;
Row: typeof TableRow;
Cell: typeof TableCell;
ColumnCellHeader: typeof TableColumnCellHeader;
+ ColumnHeader: typeof TableColumnCellHeader; // alias, back-compat with docs
}
@@
Table.Cell = TableCell;
Table.ColumnCellHeader = TableColumnCellHeader;
+Table.ColumnHeader = TableColumnCellHeader;Please also update any docs/examples or add a test to cover the alias.
Also applies to: 36-41
🏁 Script executed:
#!/bin/bash
set -e
# locate the TableComponent interface in Table.tsx
rg -n "interface TableComponent" -C3 src/components/ui/Table/Table.tsx || true
# verify existing static assignments in the component
rg -n "Table\\.ColumnCellHeader" -C2 src/components/ui/Table/Table.tsx || true
rg -n "Table\\.ColumnHeader" -C2 src/components/ui/Table/Table.tsx || true
# search documentation/examples for the alias usage
rg -n "Table\\.ColumnHeader" -t md,mdx || true
rg -n "ColumnCellHeader" -t md,mdx || trueLength of output: 705
Add Table.ColumnHeader alias for back-compat
- Expose
ColumnHeader(alias forTableColumnCellHeader) in theTableComponentinterface and as a static onTable. - Update examples/docs and add a test to cover the alias.
🤖 Prompt for AI Agents
In src/components/ui/Table/Table.tsx around lines 17 to 27, the TableComponent
type and the exported Table are missing a backward-compatible alias ColumnHeader
for TableColumnCellHeader; add ColumnHeader: typeof TableColumnCellHeader to the
TableComponent interface and attach ColumnHeader = TableColumnCellHeader as a
static property on the Table component export, then update any examples/docs
referencing ColumnHeader to use the alias and add a unit/test that asserts
Table.ColumnHeader exists and equals TableColumnCellHeader.
| const Table = React.forwardRef<HTMLTableElement, TableProps>((_props, _ref) => { | ||
| console.warn('Direct usage of Table is not supported. Please use Table.Root, Table.Head, etc. instead.'); | ||
| return null; | ||
| }; | ||
| }) as TableComponent; | ||
|
|
There was a problem hiding this comment.
Add displayName and guard console.warn to fix lint and reduce noise.
Fixes ESLint react/display-name and avoids prod console spam.
Apply:
-const Table = React.forwardRef<HTMLTableElement, TableProps>((_props, _ref) => {
- console.warn('Direct usage of Table is not supported. Please use Table.Root, Table.Head, etc. instead.');
- return null;
-}) as TableComponent;
+const Table = React.forwardRef<HTMLTableElement, TableProps>((_props, _ref) => {
+ if (process.env.NODE_ENV !== 'production') {
+ console.warn('Direct usage of Table is not supported. Please use Table.Root, Table.Head, etc. instead.');
+ }
+ return null;
+}) as TableComponent;
+Table.displayName = 'Table';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const Table = React.forwardRef<HTMLTableElement, TableProps>((_props, _ref) => { | |
| console.warn('Direct usage of Table is not supported. Please use Table.Root, Table.Head, etc. instead.'); | |
| return null; | |
| }; | |
| }) as TableComponent; | |
| const Table = React.forwardRef<HTMLTableElement, TableProps>((_props, _ref) => { | |
| if (process.env.NODE_ENV !== 'production') { | |
| console.warn('Direct usage of Table is not supported. Please use Table.Root, Table.Head, etc. instead.'); | |
| } | |
| return null; | |
| }) as TableComponent; | |
| Table.displayName = 'Table'; |
🧰 Tools
🪛 GitHub Check: lint
[failure] 30-30:
Component definition is missing display name
🪛 GitHub Actions: Lint
[error] 30-30: ESLint: react/display-name: Component definition is missing display name.
🤖 Prompt for AI Agents
In src/components/ui/Table/Table.tsx around lines 30 to 34, the forwarded Table
component is missing a displayName (triggering ESLint react/display-name) and
always calls console.warn (causing production console noise); update the
component to set Table.displayName = 'Table' after the forwardRef assignment and
wrap the console.warn call so it only runs in non-production (e.g., if
(process.env.NODE_ENV !== 'production') console.warn(...)); keep the forwardRef
return value as null and preserve the TableComponent type assertion.
1 participant
Summary
Testing
npm test src/components/ui/Table/tests/Table.test.tsxnpm run build:rolluphttps://chatgpt.com/codex/tasks/task_e_68ba8077409883318ac4dc2829fc48d3
Summary by CodeRabbit