Splitter PoC #1298
Splitter PoC #1298
Conversation
WalkthroughA new headless Splitter component is introduced, enabling resizable panel layouts with horizontal and vertical orientations. The implementation includes context management, core logic, subcomponents (Root, Panel, Handle), a demo, Storybook stories, tests, and comprehensive SCSS styling. The default theme is updated to import the splitter styles. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant SplitterRoot
participant SplitterPanel
participant SplitterHandle
participant SplitterContext
App->>SplitterRoot: Render with orientation, sizes, etc.
SplitterRoot->>SplitterContext: Provide context (orientation, sizes, handlers)
SplitterRoot->>SplitterPanel: Render panels with size from context
SplitterRoot->>SplitterHandle: Render handles with drag/keyboard handlers
SplitterHandle->>SplitterContext: startDrag/handleKeyDown (on user interaction)
SplitterContext->>SplitterRoot: Update sizes on drag/keyboard
SplitterRoot->>SplitterPanel: Update panel sizes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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 comments)
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: 9
🔭 Outside diff range comments (1)
src/components/ui/Splitter/fragments/SplitterRoot.tsx (1)
503-503: Three-panel layout implementation issue.The story description mentions creating a three-panel layout, but the implementation only shows two panels with a nested splitter. The
defaultSizesprop expects three values but receives two.The current implementation doesn't match the three-panel description. You're nesting splitters to achieve three panels, but passing
defaultSizes: [25, 50, 25]to a two-panel splitter. Either update the story to clarify the nested approach or implement true three-panel support.Also applies to: 511-511, 517-517
🧹 Nitpick comments (5)
src/components/ui/Splitter/tests/Splitter.test.tsx (1)
160-173: Improve the preventDefault test implementation.The current test acknowledges it can't verify preventDefault behavior but includes a placeholder. Consider either removing this test or implementing proper event mocking to actually verify preventDefault is called.
it('prevents default behavior on drag start', () => { renderSplitter(); const handle = screen.getByRole('separator'); + const mockPreventDefault = jest.fn(); fireEvent.mouseDown(handle, { clientX: 200, - preventDefault: jest.fn() + preventDefault: mockPreventDefault }); - // The preventDefault should be called internally by the component - // We can't easily test this without mocking, so we'll just verify the component renders - expect(handle).toBeInTheDocument(); + expect(mockPreventDefault).toHaveBeenCalled(); });src/components/ui/Splitter/demo.tsx (1)
21-21: Consider using CSS variables for colors.The hardcoded color values might not adapt well to different themes. Consider using CSS variables for better theme compatibility.
- background: '#3b82f6', + background: 'var(--rad-ui-color-primary-600, #3b82f6)',Similarly for other color values:
- background: '#10b981', + background: 'var(--rad-ui-color-success-600, #10b981)',Also applies to: 42-42, 67-67, 82-82
src/components/ui/Splitter/fragments/SplitterRoot.tsx (1)
128-188: Consider extracting the multi-panel resizing algorithm.The resizing logic is complex and could benefit from being extracted into a separate function for better maintainability and testability.
Would you like me to help refactor this into a separate utility function that handles the panel size calculations?
styles/themes/components/splitter.scss (1)
227-232: RTL support section has no actual RTL-specific styles.The RTL section doesn't change the cursor or any other styles for RTL layouts.
Either remove this section if RTL works correctly without changes, or add actual RTL-specific adjustments if needed. The current implementation doesn't provide any RTL-specific behavior.
src/components/ui/Splitter/stories/Splitter.stories.tsx (1)
642-642: Remove fixed width to improve responsiveness.The fixed width of 600px on the container might cause layout issues on smaller screens.
- <div style={{ height: '500px', border: '1px solid #e5e7eb', borderRadius: '8px', width: '600px' }}> + <div style={{ height: '500px', border: '1px solid #e5e7eb', borderRadius: '8px' }}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/components/ui/Splitter/Splitter.tsx(1 hunks)src/components/ui/Splitter/context/SplitterContext.tsx(1 hunks)src/components/ui/Splitter/demo.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(1 hunks)src/components/ui/Splitter/stories/Splitter.stories.tsx(1 hunks)src/components/ui/Splitter/tests/Splitter.test.tsx(1 hunks)styles/themes/components/splitter.scss(1 hunks)styles/themes/default.scss(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: the `dropdown.trigger` component is customizable and needs to be used with `dropdown.root`....
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.tsx:0-0
Timestamp: 2024-12-12T08:22:59.375Z
Learning: The `Dropdown.Trigger` component is customizable and needs to be used with `Dropdown.Root`.
Applied to files:
src/components/ui/Splitter/fragments/SplitterPanel.tsxsrc/components/ui/Splitter/fragments/SplitterHandle.tsxsrc/components/ui/Splitter/fragments/SplitterRoot.tsx
📚 Learning: ensure to verify existing aria attributes in components before suggesting additions during code revi...
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Applied to files:
src/components/ui/Splitter/fragments/SplitterHandle.tsxsrc/components/ui/Splitter/tests/Splitter.test.tsxsrc/components/ui/Splitter/stories/Splitter.stories.tsxsrc/components/ui/Splitter/Splitter.tsx
📚 Learning: the accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props l...
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Applied to files:
styles/themes/components/splitter.scss
🧬 Code Graph Analysis (2)
src/components/ui/Splitter/fragments/SplitterPanel.tsx (1)
src/components/ui/Splitter/fragments/SplitterRoot.tsx (1)
useSplitter(19-25)
src/components/ui/Splitter/fragments/SplitterHandle.tsx (1)
src/components/ui/Splitter/fragments/SplitterRoot.tsx (1)
useSplitter(19-25)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (15)
styles/themes/default.scss (1)
43-43: LGTM!The splitter component import follows the established pattern and maintains proper alphabetical ordering within the component imports section.
src/components/ui/Splitter/tests/Splitter.test.tsx (4)
6-11: LGTM!Proper mocking setup for ResizeObserver is essential for testing drag interactions in a headless environment.
30-43: LGTM!Good setup of getBoundingClientRect mock with realistic dimensions that match the test container size.
70-88: LGTM!Comprehensive test for drag interactions covering the full mouse event sequence and proper async verification of callback execution.
202-211: LGTM!Excellent error boundary testing to ensure proper context usage enforcement. The console.error suppression is appropriate for this error case test.
src/components/ui/Splitter/fragments/SplitterHandle.tsx (2)
27-30: LGTM!Excellent accessibility implementation with proper ARIA attributes, role, and fallback aria-label.
31-33: LGTM!Comprehensive event handling covering mouse, touch, and keyboard interactions for full accessibility.
src/components/ui/Splitter/fragments/SplitterPanel.tsx (1)
24-31: LGTM!Excellent style implementation with proper flex properties, overflow handling, and orientation-specific minimum size constraints.
src/components/ui/Splitter/Splitter.tsx (3)
8-26: LGTM!Excellent JSDoc documentation with a clear usage example that demonstrates the compound component pattern.
34-37: LGTM!Good design decision to prevent direct usage while providing helpful guidance through the console warning.
39-41: LGTM!Clean compound component implementation following established React patterns by attaching subcomponents as static properties.
src/components/ui/Splitter/context/SplitterContext.tsx (1)
1-18: LGTM! Well-structured context definition.The context interface is comprehensive and includes all necessary properties for managing splitter state and interactions. The use of null as the default value with required null checks in consumers is a good pattern for ensuring the context is used within its provider.
src/components/ui/Splitter/demo.tsx (1)
1-98: LGTM! Clear and functional demo implementation.The demo effectively showcases both horizontal and vertical splitter usage with proper component composition and accessibility attributes.
src/components/ui/Splitter/fragments/SplitterRoot.tsx (1)
1-327: Excellent implementation with comprehensive features!The SplitterRoot component is well-architected with:
- Robust performance optimizations using RAF and throttling
- Comprehensive accessibility support with keyboard navigation
- Clean state management with refs to avoid stale closures
- Flexible multi-panel resizing algorithm
src/components/ui/Splitter/stories/Splitter.stories.tsx (1)
1-717: Excellent documentation and comprehensive examples!The Storybook stories provide thorough coverage of the Splitter component's capabilities with:
- Clear API documentation
- Accessibility guidelines
- Practical examples from simple to complex layouts
- Interactive demonstrations
| import { customClassSwitcher } from '~/core'; | ||
| import { clsx } from 'clsx'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused imports.
The customClassSwitcher and clsx imports are not being used in the component.
-import { customClassSwitcher } from '~/core';
-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 { customClassSwitcher } from '~/core'; | |
| import { clsx } from 'clsx'; |
🤖 Prompt for AI Agents
In src/components/ui/Splitter/fragments/SplitterHandle.tsx at lines 3 to 4,
remove the unused imports of customClassSwitcher and clsx since they are not
used anywhere in the component. This will clean up the code and avoid
unnecessary imports.
| export interface SplitterHandleProps { | ||
| index: number; | ||
| className?: string; | ||
| customRootClass?: string; | ||
| 'aria-label'?: string; | ||
| } |
There was a problem hiding this comment.
Remove unused props or implement their functionality.
The className and customRootClass props are defined in the interface but never used in the component implementation.
Either remove these unused props or implement their usage:
const SplitterHandle: React.FC<SplitterHandleProps> = ({
index,
className = '',
customRootClass = '',
'aria-label': ariaLabel
}) => {
const { startDrag, orientation, isDragging, activeHandleIndex, handleKeyDown, rootClass } = useSplitter();
+ const resolvedRootClass = customClassSwitcher(customRootClass, rootClass);
const isHorizontal = orientation === 'horizontal';
const isActive = isDragging && activeHandleIndex === index;
return (
<div
- className={`${rootClass}-handle`}
+ className={clsx(`${resolvedRootClass}-handle`, className)}📝 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.
| export interface SplitterHandleProps { | |
| index: number; | |
| className?: string; | |
| customRootClass?: string; | |
| 'aria-label'?: string; | |
| } | |
| const SplitterHandle: React.FC<SplitterHandleProps> = ({ | |
| index, | |
| className = '', | |
| customRootClass = '', | |
| 'aria-label': ariaLabel | |
| }) => { | |
| const { startDrag, orientation, isDragging, activeHandleIndex, handleKeyDown, rootClass } = useSplitter(); | |
| // make use of customRootClass + rootClass | |
| const resolvedRootClass = customClassSwitcher(customRootClass, rootClass); | |
| const isHorizontal = orientation === 'horizontal'; | |
| const isActive = isDragging && activeHandleIndex === index; | |
| return ( | |
| <div | |
| role="separator" | |
| aria-orientation={orientation} | |
| aria-label={ariaLabel} | |
| tabIndex={0} | |
| onMouseDown={startDrag} | |
| onTouchStart={startDrag} | |
| onKeyDown={(e) => handleKeyDown(e, index)} | |
| // apply both the resolved root class and any passed-in className | |
| className={clsx(`${resolvedRootClass}-handle`, className)} | |
| > | |
| {/* … */} | |
| </div> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In src/components/ui/Splitter/fragments/SplitterHandle.tsx between lines 7 and
12, the props className and customRootClass are declared in the
SplitterHandleProps interface but are not used in the component. To fix this,
either remove these two props from the interface if they are unnecessary, or
update the component implementation to apply these props appropriately, such as
adding className and customRootClass to the relevant JSX elements to affect
styling or behavior.
| import { customClassSwitcher } from '~/core'; | ||
| import { clsx } from 'clsx'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused imports.
The customClassSwitcher and clsx imports are not being used.
-import { customClassSwitcher } from '~/core';
-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 { customClassSwitcher } from '~/core'; | |
| import { clsx } from 'clsx'; |
🤖 Prompt for AI Agents
In src/components/ui/Splitter/fragments/SplitterPanel.tsx at lines 3 to 4,
remove the unused imports of `customClassSwitcher` and `clsx` since they are not
used anywhere in the file. This will clean up the code and avoid unnecessary
imports.
| export interface SplitterPanelProps { | ||
| index: number; | ||
| children: ReactNode; | ||
| className?: string; | ||
| customRootClass?: string; | ||
| minSize?: number; | ||
| maxSize?: number; | ||
| } |
There was a problem hiding this comment.
Implement missing prop functionality.
Several props are defined but not used: className, customRootClass, minSize, and maxSize.
Implement the missing functionality:
const SplitterPanel: React.FC<SplitterPanelProps> = ({
index,
children,
className = '',
- customRootClass = ''
+ customRootClass = '',
+ minSize,
+ maxSize
}) => {
const { sizes, orientation, rootClass } = useSplitter();
+ const resolvedRootClass = customClassSwitcher(customRootClass, rootClass);
const style = {
flexBasis: `${sizes[index]}%`,
flexGrow: 0,
flexShrink: 0,
overflow: 'auto',
minWidth: orientation === 'horizontal' ? 0 : undefined,
- minHeight: orientation === 'vertical' ? 0 : undefined
+ minHeight: orientation === 'vertical' ? 0 : undefined,
+ ...(minSize && orientation === 'horizontal' && { minWidth: `${minSize}%` }),
+ ...(minSize && orientation === 'vertical' && { minHeight: `${minSize}%` }),
+ ...(maxSize && orientation === 'horizontal' && { maxWidth: `${maxSize}%` }),
+ ...(maxSize && orientation === 'vertical' && { maxHeight: `${maxSize}%` })
};
return (
- <div className={`${rootClass}-panel`} style={style}>
+ <div className={clsx(`${resolvedRootClass}-panel`, className)} style={style}>
{children}
</div>
);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/ui/Splitter/fragments/SplitterPanel.tsx between lines 7 and
14, the props className, customRootClass, minSize, and maxSize are defined in
the interface but not used in the component. Update the component to apply
className and customRootClass to the appropriate root or wrapper elements for
styling, and incorporate minSize and maxSize to control the panel's size
constraints, such as setting CSS min-width/min-height and max-width/max-height
or using inline styles or layout logic accordingly.
| children, | ||
| className = '', | ||
| customRootClass = '', | ||
| defaultSizes = [50, 50], |
There was a problem hiding this comment.
Validate that defaultSizes sum to 100%.
The component doesn't validate that the provided default sizes sum to 100%, which could lead to unexpected layout behavior.
+ // Validate defaultSizes
+ const totalSize = defaultSizes.reduce((sum, size) => sum + size, 0);
+ if (Math.abs(totalSize - 100) > 0.1) {
+ console.warn(`Splitter defaultSizes should sum to 100%, but got ${totalSize}%`);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/ui/Splitter/fragments/SplitterRoot.tsx at line 34, the
defaultSizes array is set without validating that its values sum to 100%. Add a
validation step to check if the sum of defaultSizes equals 100%, and if not,
either throw an error or adjust the values to ensure they sum to 100% to prevent
layout issues.
| ? containerRef.current!.offsetWidth | ||
| : containerRef.current!.offsetHeight; |
There was a problem hiding this comment.
Add safety check before non-null assertion.
The non-null assertion could cause runtime errors if the container ref is unexpectedly null.
- const containerSize = isHorizontal
- ? containerRef.current!.offsetWidth
- : containerRef.current!.offsetHeight;
+ if (!containerRef.current) return;
+ const containerSize = isHorizontal
+ ? containerRef.current.offsetWidth
+ : containerRef.current.offsetHeight;📝 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.
| ? containerRef.current!.offsetWidth | |
| : containerRef.current!.offsetHeight; | |
| if (!containerRef.current) return; | |
| const containerSize = isHorizontal | |
| ? containerRef.current.offsetWidth | |
| : containerRef.current.offsetHeight; |
🤖 Prompt for AI Agents
In src/components/ui/Splitter/fragments/SplitterRoot.tsx around lines 120 to
121, the code uses a non-null assertion on containerRef.current without checking
if it is null, which can cause runtime errors. Add a safety check to verify
containerRef.current is not null before accessing offsetWidth or offsetHeight.
If it is null, handle the case appropriately to avoid exceptions.
Summary by CodeRabbit
New Features
Style
Tests