refactor: forward refs in Splitter #1461
Conversation
WalkthroughRefactors Splitter components to use React.forwardRef, broaden props to standard div attributes, and expose static subcomponents on the main Splitter export. Implements ref forwarding for Root, Panel, and Handle, adds displayName assignments, merges styles/props, and adds a test verifying refs are forwarded to DOM elements. Changes
Sequence Diagram(s)sequenceDiagram
actor App
participant Splitter as Splitter
participant Root as Splitter.Root (div)
participant Panel as Splitter.Panel (div)
participant Handle as Splitter.Handle (div)
App->>Splitter: Render with children
Splitter->>Root: Render, forward ref
Splitter->>Panel: Render, forward ref
Splitter->>Handle: Render, forward ref
Root-->>App: rootRef.current = HTMLDivElement
Panel-->>App: panelRef.current = HTMLDivElement
Handle-->>App: handleRef.current = HTMLDivElement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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: 4
🧹 Nitpick comments (6)
src/components/ui/Splitter/tests/Splitter.test.tsx (2)
212-235: Good ref-forwarding coverage; tighten assertions to match actual nodes.Consider also asserting the refs point to the exact rendered elements (not just instances). This makes regressions easier to catch.
Apply this diff to extend the test:
expect(rootRef.current).toBeInstanceOf(HTMLDivElement); expect(panelRef.current).toBeInstanceOf(HTMLDivElement); expect(handleRef.current).toBeInstanceOf(HTMLDivElement); + // Stronger equality checks + const panel0Parent = screen.getByTestId('panel-0').parentElement as HTMLDivElement; + expect(panelRef.current).toBe(panel0Parent); + const handle = screen.getByRole('separator') as HTMLDivElement; + expect(handleRef.current).toBe(handle);
1-4: Rollup OOM during build: practical mitigations.
- Externalize peer deps in Rollup (react, react-dom, clsx) to shrink bundle graph.
- Disable or limit sourcemaps for production builds.
- Switch to @rollup/plugin-typescript with transpileOnly or use esbuild plugin for ts/tsx.
- Run build with NODE_OPTIONS="--max-old-space-size=4096".
- Ensure tree-shaking enabled and avoid bundling tests/stories via include/exclude globs.
src/components/ui/Splitter/fragments/SplitterHandle.tsx (2)
16-17: Remove or surface “active” state.isActive is computed but unused. Either remove it or expose it for styling/testing.
Apply this diff to surface as a data attribute:
- const isActive = isDragging && activeHandleIndex === index; + const isActive = isDragging && activeHandleIndex === index; @@ - className={clsx(`${rootClass}-handle`, className)} + className={clsx(`${rootClass}-handle`, className)} + data-active={isActive || undefined}
6-9: Consider dropping unused prop from the public API.customRootClass isn’t used. Keeping unused public props is confusing and adds maintenance overhead.
Apply this diff:
-export interface SplitterHandleProps extends React.ComponentPropsWithoutRef<'div'> { +export interface SplitterHandleProps extends React.ComponentPropsWithoutRef<'div'> { index: number; - customRootClass?: string; }src/components/ui/Splitter/fragments/SplitterPanel.tsx (1)
6-11: Public props not used here—verify or trim.customRootClass, minSize, and maxSize aren’t referenced in this component. If Root consumes them via context, fine; otherwise consider removing to keep API lean.
Would you like me to scan the repo to confirm these props are actually used by SplitterRoot or elsewhere?
src/components/ui/Splitter/Splitter.tsx (1)
38-41: Consider removing or conditionalizing the console warning.While the warning is helpful during development, it will appear in production builds. Consider either:
- Removing it entirely if direct usage is truly unsupported
- Wrapping it in a development-only check
- Throwing an error instead if this is a critical usage error
const Splitter = React.forwardRef<React.ElementRef<'div'>, SplitterProps>((props, ref) => { - console.warn('Direct usage of Splitter is not supported. Please use Splitter.Root, Splitter.Panel, etc. instead.'); + if (process.env.NODE_ENV !== 'production') { + console.warn('Direct usage of Splitter is not supported. Please use Splitter.Root, Splitter.Panel, etc. instead.'); + } return <div ref={ref} {...props} />; }) as SplitterComponent;
📜 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 (5)
src/components/ui/Splitter/Splitter.tsx(1 hunks)src/components/ui/Splitter/fragments/SplitterHandle.tsx(1 hunks)src/components/ui/Splitter/fragments/SplitterPanel.tsx(1 hunks)src/components/ui/Splitter/fragments/SplitterRoot.tsx(3 hunks)src/components/ui/Splitter/tests/Splitter.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/ui/Splitter/fragments/SplitterPanel.tsx (1)
src/components/ui/Splitter/fragments/SplitterRoot.tsx (1)
useSplitter(17-23)
src/components/ui/Splitter/fragments/SplitterHandle.tsx (1)
src/components/ui/Splitter/fragments/SplitterRoot.tsx (1)
useSplitter(17-23)
⏰ 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 (9)
src/components/ui/Splitter/fragments/SplitterPanel.tsx (1)
13-16: Ref-forwarding and prop spreading look good.Signature, ref wiring, style merging, and displayName assignment are correct.
Also applies to: 29-38
src/components/ui/Splitter/Splitter.tsx (2)
28-29: LGTM! Clean type definition for forwardRef props.The use of
React.ComponentPropsWithoutRef<'div'>properly extends all standard div attributes while excluding the ref, which will be handled separately by forwardRef.
30-36: Well-structured compound component type definition.The type correctly combines the ForwardRefExoticComponent with static members for the compound component pattern. This provides excellent TypeScript support for the
Splitter.Root,Splitter.Panel, andSplitter.HandleAPI.src/components/ui/Splitter/fragments/SplitterRoot.tsx (6)
7-14: Clean interface extension with ComponentPropsWithoutRef.The interface properly extends div props, allowing all standard HTML div attributes to be passed through, which improves component flexibility and reusability.
27-41: Proper forwardRef implementation with correct type annotations.The forwardRef setup correctly uses
React.ElementRef<'div'>for the ref type and properly destructures props including the newstyleprop.
49-56: Robust ref merging implementation.The
mergedRefcallback correctly handles both function refs and object refs, ensuring that both the internalcontainerRefand the forwarded ref receive the DOM node. This is essential for maintaining internal functionality while exposing the ref to parent components.
308-317: Excellent prop spreading and style merging.The implementation correctly:
- Spreads the remaining props first
- Then applies the ref and className
- Properly merges the incoming style prop with the component's required styles
This order ensures that the component's essential styles are preserved while allowing customization.
325-325: Good practice: displayName for debugging.Setting the displayName helps with debugging in React DevTools and error messages.
43-43: No changes needed—null-check guards ensure safe non‐null usage
Theif (!containerRef.current) return;at line 125 preventscontainerRef.currentfrom ever being null when using the non-null assertions on lines 141–142.
| 'use client'; | ||
| import React from 'react'; | ||
| import { customClassSwitcher } from '~/core'; | ||
| import { clsx } from 'clsx'; |
There was a problem hiding this comment.
Fix incorrect clsx import.
clsx has a default export. Named import will break at runtime.
Apply this diff:
-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/Splitter/fragments/SplitterHandle.tsx around line 3, the
code imports clsx using a named import which is incorrect because clsx uses a
default export; replace the named import with a default import (e.g., import
clsx from 'clsx') so runtime errors are avoided, and ensure any existing usages
of clsx in the file remain unchanged.
| {...props} | ||
| ref={forwardedRef} | ||
| className={clsx(`${rootClass}-handle`, className)} | ||
| role="separator" | ||
| aria-orientation={orientation} | ||
| aria-label={ariaLabel || `${orientation} resize handle`} | ||
| tabIndex={0} | ||
| onMouseDown={(e) => startDrag(index, e)} | ||
| onTouchStart={(e) => startDrag(index, e)} | ||
| onKeyDown={(e) => handleKeyDown(index, e)} | ||
|
|
||
| style={style} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t overwrite consumer event handlers; compose them with internal ones.
Current {...props} order causes any passed onMouseDown/onTouchStart/onKeyDown to be silently overridden. Compose them so analytics and side-effects still fire.
Apply this diff:
->(({ index, className, 'aria-label': ariaLabel, style, ...props }, forwardedRef) => {
+>(({ index, className, 'aria-label': ariaLabel, style, onMouseDown: userOnMouseDown, onTouchStart: userOnTouchStart, onKeyDown: userOnKeyDown, ...props }, forwardedRef) => {
@@
- <div
- {...props}
- ref={forwardedRef}
+ <div
+ {...props}
+ ref={forwardedRef}
className={clsx(`${rootClass}-handle`, className)}
role="separator"
aria-orientation={orientation}
aria-label={ariaLabel || `${orientation} resize handle`}
tabIndex={0}
- onMouseDown={(e) => startDrag(index, e)}
- onTouchStart={(e) => startDrag(index, e)}
- onKeyDown={(e) => handleKeyDown(index, e)}
+ onMouseDown={(e) => {
+ userOnMouseDown?.(e);
+ startDrag(index, e);
+ }}
+ onTouchStart={(e) => {
+ userOnTouchStart?.(e);
+ startDrag(index, e);
+ }}
+ onKeyDown={(e) => {
+ userOnKeyDown?.(e);
+ handleKeyDown(index, e);
+ }}
style={style}
/>📝 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.
| {...props} | |
| ref={forwardedRef} | |
| className={clsx(`${rootClass}-handle`, className)} | |
| role="separator" | |
| aria-orientation={orientation} | |
| aria-label={ariaLabel || `${orientation} resize handle`} | |
| tabIndex={0} | |
| onMouseDown={(e) => startDrag(index, e)} | |
| onTouchStart={(e) => startDrag(index, e)} | |
| onKeyDown={(e) => handleKeyDown(index, e)} | |
| style={style} | |
| /> | |
| // File: src/components/ui/Splitter/fragments/SplitterHandle.tsx | |
| // Lines: 20-31 | |
| (({ | |
| index, | |
| className, | |
| 'aria-label': ariaLabel, | |
| style, | |
| onMouseDown: userOnMouseDown, | |
| onTouchStart: userOnTouchStart, | |
| onKeyDown: userOnKeyDown, | |
| ...props | |
| }, forwardedRef) => { | |
| return ( | |
| <div | |
| {...props} | |
| ref={forwardedRef} | |
| className={clsx(`${rootClass}-handle`, className)} | |
| role="separator" | |
| aria-orientation={orientation} | |
| aria-label={ariaLabel || `${orientation} resize handle`} | |
| tabIndex={0} | |
| onMouseDown={(e) => { | |
| userOnMouseDown?.(e); | |
| startDrag(index, e); | |
| }} | |
| onTouchStart={(e) => { | |
| userOnTouchStart?.(e); | |
| startDrag(index, e); | |
| }} | |
| onKeyDown={(e) => { | |
| userOnKeyDown?.(e); | |
| handleKeyDown(index, e); | |
| }} | |
| style={style} | |
| /> | |
| ); | |
| }) |
🤖 Prompt for AI Agents
In src/components/ui/Splitter/fragments/SplitterHandle.tsx around lines 20–31,
the spread {...props} followed by internal onMouseDown/onTouchStart/onKeyDown
handlers silently overrides consumer handlers; fix by composing the handlers
instead: read any existing props.onMouseDown/onTouchStart/onKeyDown into local
constants and create composed handlers that call the consumer handler (if
present) and then the internal startDrag/handleKeyDown (or the other order if
you prefer consumer first), passing the original event through and preserving
return values/stopPropagation behavior; replace the hardwired
onMouseDown/onTouchStart/onKeyDown props with these composed functions so
consumer analytics/side-effects still run.
| import React, { ReactNode } from 'react'; | ||
| import { customClassSwitcher } from '~/core'; | ||
| import React from 'react'; | ||
| import { clsx } from 'clsx'; |
There was a problem hiding this comment.
Fix incorrect clsx import.
Use default import to avoid runtime failure.
Apply this diff:
-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/Splitter/fragments/SplitterPanel.tsx around line 3, the
file currently imports clsx as a named import which causes a runtime failure;
change the import to use the default export from 'clsx' (replace the named
import with a default import) so existing usages of clsx work at runtime.
| const panelStyle = { | ||
| flexBasis: `${sizes[index]}%`, | ||
| flexGrow: 0, | ||
| flexShrink: 0, | ||
| overflow: 'auto', | ||
| minWidth: orientation === 'horizontal' ? 0 : undefined, | ||
| minHeight: orientation === 'vertical' ? 0 : undefined | ||
| }; | ||
| minHeight: orientation === 'vertical' ? 0 : undefined, | ||
| ...style | ||
| } as React.CSSProperties; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against undefined sizes[index] to avoid “undefined%”.
If sizes[index] is missing, flexBasis becomes "undefined%". Guard and only set when defined.
Apply this diff:
- const panelStyle = {
- flexBasis: `${sizes[index]}%`,
+ const basis = sizes[index];
+ const panelStyle: React.CSSProperties = {
+ ...(typeof basis === 'number' ? { flexBasis: `${basis}%` } : {}),
flexGrow: 0,
flexShrink: 0,
overflow: 'auto',
minWidth: orientation === 'horizontal' ? 0 : undefined,
- minHeight: orientation === 'vertical' ? 0 : undefined,
- ...style
- } as React.CSSProperties;
+ minHeight: orientation === 'vertical' ? 0 : undefined,
+ ...style
+ };📝 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 panelStyle = { | |
| flexBasis: `${sizes[index]}%`, | |
| flexGrow: 0, | |
| flexShrink: 0, | |
| overflow: 'auto', | |
| minWidth: orientation === 'horizontal' ? 0 : undefined, | |
| minHeight: orientation === 'vertical' ? 0 : undefined | |
| }; | |
| minHeight: orientation === 'vertical' ? 0 : undefined, | |
| ...style | |
| } as React.CSSProperties; | |
| const basis = sizes[index]; | |
| const panelStyle: React.CSSProperties = { | |
| ...(typeof basis === 'number' ? { flexBasis: `${basis}%` } : {}), | |
| flexGrow: 0, | |
| flexShrink: 0, | |
| overflow: 'auto', | |
| minWidth: orientation === 'horizontal' ? 0 : undefined, | |
| minHeight: orientation === 'vertical' ? 0 : undefined, | |
| ...style | |
| }; |
🤖 Prompt for AI Agents
In src/components/ui/Splitter/fragments/SplitterPanel.tsx around lines 19 to 27,
the panelStyle builds flexBasis using `${sizes[index]}%` which yields
"undefined%" when sizes[index] is missing; change the logic to only set
flexBasis when sizes[index] is a defined number (e.g., compute a variable like
const flexBasis = typeof sizes[index] === 'number' ? `${sizes[index]}%` :
undefined and include flexBasis in the style object conditionally) so that
flexBasis is omitted when the size is undefined, preserving types as
React.CSSProperties.
kotAPI
deleted the
kotapi/refactor-splitter-component-with-forwardref-o555vb
branch
Summary
Testing
npm testnpm run build:rollup(fails: FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory)https://chatgpt.com/codex/tasks/task_e_68ba8070cc4c83318da638a5fffd6722
Summary by CodeRabbit
New Features
Refactor
Tests