Utility hooks additions #867
Conversation
WalkthroughThe changes introduce a new custom hook, Changes
Sequence Diagram(s)sequenceDiagram
participant Component as React Component
participant Hook as useLayoutEffect Hook
participant Check as Browser Environment Check
Component->>Hook: Invoke useLayoutEffect
Hook->>Check: Check for globalThis.document
alt Document exists (Browser)
Check-->>Hook: Document found
Hook->>Component: Execute React's useLayoutEffect
else Document not found (SSR)
Check-->>Hook: No document detected
Hook->>Component: Return no-operation
end
Poem
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 1
🧹 Nitpick comments (2)
src/core/hooks/useLayoutEffect/index.tsx (1)
10-10: Consider adding TypeScript types for better type safety.While the implementation is correct and follows Radix UI's approach, adding explicit types would improve type safety and documentation.
-const useLayoutEffect = globalThis.document ? ReactUseLayoutEffect : () => {}; +const useLayoutEffect: typeof ReactUseLayoutEffect = + globalThis.document ? ReactUseLayoutEffect : () => {};src/core/hooks/useLayoutEffect/useLayoutEffect.test.tsx (1)
7-9: Add meaningful effect logic or remove empty comment.The empty comment "mounts" doesn't provide value. Either add meaningful effect logic for testing or remove the comment.
useLayoutEffect(() => { - // mounts + // Effect runs after layout changes + // Left empty as we're just testing mounting behavior }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/hooks/useLayoutEffect/index.tsx(1 hunks)src/core/hooks/useLayoutEffect/useLayoutEffect.test.tsx(1 hunks)
🔇 Additional comments (1)
src/core/hooks/useLayoutEffect/index.tsx (1)
3-8: LGTM! Clear and helpful comments.The comments effectively explain the purpose and behavior of this SSR-compatible implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/core/primitives/Toggle/index.tsx (1)
34-44: Fix double composition of event handlers.The
handleTogglePressedhandler is composed twice:
- In its definition with
props.onClick- Again in the button's
onClickpropApply this fix to avoid double composition:
- const handleTogglePressed = composeEventHandlers(props.onClick, () => { + const handleTogglePressed = () => { if (disabled) { return; } const updatedPressed = !isPressed; if (!isControlled) { setUncontrolledPressed(updatedPressed); } onPressedChange(updatedPressed); - }); + }; return <Primitive.button onClick={composeEventHandlers(props.onClick, handleTogglePressed)} onKeyDown={composeEventHandlers(props.onKeyDown, handleKeyDown)}Also applies to: 58-59
🧹 Nitpick comments (4)
src/core/hooks/composeEventHandlers/index.tsx (1)
8-20: Consider a more concise implementation.The current implementation is correct but could be more concise and avoid the early return.
Consider this alternative implementation:
- return function handleEvent(event: E) { - // If the original event handler is defined, call it - if (typeof originalEventHandler === 'function') { - originalEventHandler(event); - } - - // If the default prevented flag is false or the event is not prevented, call our event handler - if (checkForDefaultPrevented === false || !event.defaultPrevented) { - return ourEventHandler?.(event); - } - }; + return function handleEvent(event: E) { + originalEventHandler?.(event); + if (checkForDefaultPrevented === false || !event.defaultPrevented) { + ourEventHandler?.(event); + } + };src/core/hooks/composeEventHandlers/composeEventHandlers.test.tsx (3)
5-28: Improve test component focus and remove console.log statements.The test component could be more focused and avoid using console.log for assertions.
Consider this alternative implementation:
const OnlyOriginalHandlerWithPreventDefault = ({ checkForDefaultPrevented = true }: { checkForDefaultPrevented?: boolean; + onOriginalClick?: () => void; + onOurClick?: () => void; }) => { const originalClickHandler = (event: React.MouseEvent<HTMLButtonElement>) => { event.preventDefault(); - // we prevent default, so we should not see our handler - console.log('RETURNING_ORIGINAL_HANDLER'); + onOriginalClick?.(); }; const ourClickHandler = () => { - // This won't be triggered because we prevent default in the original handler - console.log('RETURNING_OUR_HANDLER'); - - console.log('RETURN_OUR_HANDLER_WITH_CHECK_FOR_DEFAULT_PREVENTED'); + onOurClick?.(); };
30-39: Simplify test setup by removing console spies.With the proposed component changes, the test setup can be simplified by removing console spies.
Consider this alternative setup:
describe('composeEventHandlers', () => { - let consoleLogSpy: jest.SpyInstance; - - beforeEach(() => { - consoleLogSpy = jest.spyOn(console, 'log').mockImplementation(() => {}); - }); - - afterEach(() => { - consoleLogSpy.mockRestore(); - });
41-58: Add test coverage for edge cases.The current tests cover the main functionality but could benefit from additional edge cases.
Consider adding these test cases:
test('should handle undefined handlers gracefully', () => { const onOriginalClick = jest.fn(); render( <OnlyOriginalHandlerWithPreventDefault onOriginalClick={onOriginalClick} /> ); const button = screen.getByText('Click Me'); fireEvent.click(button); expect(onOriginalClick).toHaveBeenCalled(); }); test('should handle null handlers gracefully', () => { const onOurClick = jest.fn(); const Component = () => ( <button onClick={composeEventHandlers( null as any, onOurClick )} > Click Me </button> ); render(<Component />); const button = screen.getByText('Click Me'); fireEvent.click(button); expect(onOurClick).toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/hooks/composeEventHandlers/composeEventHandlers.test.tsx(1 hunks)src/core/hooks/composeEventHandlers/index.tsx(1 hunks)src/core/hooks/useLayoutEffect/index.tsx(1 hunks)src/core/primitives/Toggle/index.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/hooks/useLayoutEffect/index.tsx
🧰 Additional context used
📓 Learnings (1)
src/core/primitives/Toggle/index.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
src/core/hooks/composeEventHandlers/index.tsx (2)
1-1: LGTM! Well-defined type with proper generic constraint.The
EventHandlertype is correctly defined with a generic parameter defaulting toReact.SyntheticEvent.
3-7: LGTM! Well-structured function signature with proper typing.The function signature is well-designed with:
- Proper generic constraint
- Optional parameters
- Type-safe options object with default value
src/core/primitives/Toggle/index.tsx (1)
11-12: LGTM! Well-typed event handler props.The new event handler props follow React's naming convention and are correctly typed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/components/layout/ScrollContainers/FullHeightScroll.tsx (1)
3-5: Remove empty style object and clean up className template literal.The empty style object and extra spaces in the className template literal should be cleaned up.
- return <div className={` ${className} ${scrollable?' overflow-y-auto overflow-x-hidden':''}`} {...props} style={{ - - }}> + return <div className={`${className} ${scrollable ? ' overflow-y-auto overflow-x-hidden' : ''}`} {...props}>docs/app/docs/layout.tsx (1)
5-7: Add ARIA label to InfoIcon SVG.Consider adding aria-label to improve accessibility of the InfoIcon SVG.
const InfoIcon = () => { - return (<svg width="18" height="18" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M7.49991 0.876892C3.84222 0.876892 0.877075 3.84204 0.877075 7.49972C0.877075 11.1574 3.84222 14.1226 7.49991 14.1226C11.1576 14.1226 14.1227 11.1574 14.1227 7.49972C14.1227 3.84204 11.1576 0.876892 7.49991 0.876892ZM1.82707 7.49972C1.82707 4.36671 4.36689 1.82689 7.49991 1.82689C10.6329 1.82689 13.1727 4.36671 13.1727 7.49972C13.1727 10.6327 10.6329 13.1726 7.49991 13.1726C4.36689 13.1726 1.82707 10.6327 1.82707 7.49972ZM8.24992 4.49999C8.24992 4.9142 7.91413 5.24999 7.49992 5.24999C7.08571 5.24999 6.74992 4.9142 6.74992 4.49999C6.74992 4.08577 7.08571 3.74999 7.49992 3.74999C7.91413 3.74999 8.24992 4.08577 8.24992 4.49999ZM6.00003 5.99999H6.50003H7.50003C7.77618 5.99999 8.00003 6.22384 8.00003 6.49999V9.99999H8.50003H9.00003V11H8.50003H7.50003H6.50003H6.00003V9.99999H6.50003H7.00003V6.99999H6.50003H6.00003V5.99999Z" fill="currentColor" fillRule="evenodd" clipRule="evenodd"></path></svg>); + return (<svg aria-label="Information" role="img" width="18" height="18" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M7.49991 0.876892C3.84222 0.876892 0.877075 3.84204 0.877075 7.49972C0.877075 11.1574 3.84222 14.1226 7.49991 14.1226C11.1576 14.1226 14.1227 11.1574 14.1227 7.49972C14.1227 3.84204 11.1576 0.876892 7.49991 0.876892ZM1.82707 7.49972C1.82707 4.36671 4.36689 1.82689 7.49991 1.82689C10.6329 1.82689 13.1727 4.36671 13.1727 7.49972C13.1727 10.6327 10.6329 13.1726 7.49991 13.1726C4.36689 13.1726 1.82707 10.6327 1.82707 7.49972ZM8.24992 4.49999C8.24992 4.9142 7.91413 5.24999 7.49992 5.24999C7.08571 5.24999 6.74992 4.9142 6.74992 4.49999C6.74992 4.08577 7.08571 3.74999 7.49992 3.74999C7.91413 3.74999 8.24992 4.08577 8.24992 4.49999ZM6.00003 5.99999H6.50003H7.50003C7.77618 5.99999 8.00003 6.22384 8.00003 6.49999V9.99999H8.50003H9.00003V11H8.50003H7.50003H6.50003H6.00003V9.99999H6.50003H7.00003V6.99999H6.50003H6.00003V5.99999Z" fill="currentColor" fillRule="evenodd" clipRule="evenodd"></path></svg>);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/app/docs/layout.tsx(1 hunks)docs/app/testing/full_page_scroll/page.tsx(1 hunks)docs/components/layout/Documentation/Documentation.js(1 hunks)docs/components/layout/ScrollContainers/FullHeightScroll.tsx(1 hunks)src/core/hooks/composeEventHandlers/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/hooks/composeEventHandlers/index.tsx
🔇 Additional comments (5)
docs/components/layout/Documentation/Documentation.js (2)
49-49: Document the color prop and ensure consistent Table usage.The Table component is used with different props across the file. Consider:
- Adding documentation for the new color prop and its possible values
- Applying consistent styling by adding the color prop to the Table in KeyboardShortcuts component (line 89)
// In KeyboardShortcuts component - <Table columns={keyboardShortcuts.columns} data={keyboardShortcuts.data} > + <Table columns={keyboardShortcuts.columns} data={keyboardShortcuts.data} color="gray" >
49-49: Verify type definitions for the Table component.Let's ensure the color prop is properly typed in the Table component.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for Table component type definitions and documentation echo "Searching for Table component type definitions..." fd -e ts -e tsx -e d.ts Table echo "Searching for Table component documentation..." fd -e md TableLength of output: 688
Verified: Table component's type definitions include the "color" prop.
We confirmed that the Table component is defined in "src/components/ui/Table/Table.tsx" with explicit type definitions that include the "color" prop. No further documentation issues were found.
docs/components/layout/ScrollContainers/FullHeightScroll.tsx (1)
1-1: LGTM! Props signature update looks good.The addition of className prop with a default value and props spreading follows React best practices.
docs/app/testing/full_page_scroll/page.tsx (1)
4-14: LGTM! Component changes look good.The implementation properly demonstrates the usage of the new className prop and maintains good practices with key usage in the map function.
docs/app/docs/layout.tsx (1)
17-17: LGTM! Layout improvements look good.The max-width constraint and auto margins improve content layout consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/hooks/useLayoutEffect/useLayoutEffect.test.tsx (2)
5-11: Improve test component documentation and clarity.The test component could be more explicit about what it's testing:
- Remove redundant comments
- Add meaningful documentation or make the test intention clear through code
const TestComponent = () => { - // invoke useLayoutEffect and check if the component still mounts as expected useLayoutEffect(() => { - // mounts + // Intentionally empty for SSR compatibility test }, []); return <div>Hello</div>; };
14-16: Add assertions to verify component rendering.While the test verifies that the component mounts without errors, it would be more robust to assert that the component actually rendered its content.
it('should mount without errors in SSR environment', () => { - render(<TestComponent />); + const { getByText } = render(<TestComponent />); + expect(getByText('Hello')).toBeInTheDocument(); });
| it('should execute effect in browser environment', () => { | ||
| const mockFn = jest.fn(); | ||
| const TestComponent = () => { | ||
| useLayoutEffect(() => { | ||
| mockFn(); | ||
| }, []); | ||
| return <div>Hello</div>; | ||
| }; | ||
|
|
||
| // Mock document existence | ||
| const originalDocument = global.document; | ||
| global.document = {} as typeof document; | ||
|
|
||
| render(<TestComponent />); | ||
| expect(mockFn).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Cleanup | ||
| global.document = originalDocument; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve browser environment test robustness.
The browser environment test could be more robust with these improvements:
- Use proper test lifecycle hooks for setup/cleanup
- Use a more realistic document mock
- Add error handling for cleanup
+ let originalDocument: typeof document;
+
+ beforeEach(() => {
+ originalDocument = global.document;
+ // Mock a more realistic document object
+ global.document = {
+ createElement: jest.fn(),
+ body: {},
+ } as unknown as typeof document;
+ });
+
+ afterEach(() => {
+ try {
+ global.document = originalDocument;
+ } catch (error) {
+ console.error('Failed to restore document:', error);
+ throw error;
+ }
+ });
+
it('should execute effect in browser environment', () => {
const mockFn = jest.fn();
const TestComponent = () => {
useLayoutEffect(() => {
mockFn();
}, []);
return <div>Hello</div>;
};
- // Mock document existence
- const originalDocument = global.document;
- global.document = {} as typeof document;
-
render(<TestComponent />);
expect(mockFn).toHaveBeenCalledTimes(1);
-
- // Cleanup
- global.document = originalDocument;
});📝 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.
| it('should execute effect in browser environment', () => { | |
| const mockFn = jest.fn(); | |
| const TestComponent = () => { | |
| useLayoutEffect(() => { | |
| mockFn(); | |
| }, []); | |
| return <div>Hello</div>; | |
| }; | |
| // Mock document existence | |
| const originalDocument = global.document; | |
| global.document = {} as typeof document; | |
| render(<TestComponent />); | |
| expect(mockFn).toHaveBeenCalledTimes(1); | |
| // Cleanup | |
| global.document = originalDocument; | |
| }); | |
| let originalDocument: typeof document; | |
| beforeEach(() => { | |
| originalDocument = global.document; | |
| // Mock a more realistic document object | |
| global.document = { | |
| createElement: jest.fn(), | |
| body: {}, | |
| } as unknown as typeof document; | |
| }); | |
| afterEach(() => { | |
| try { | |
| global.document = originalDocument; | |
| } catch (error) { | |
| console.error('Failed to restore document:', error); | |
| throw error; | |
| } | |
| }); | |
| it('should execute effect in browser environment', () => { | |
| const mockFn = jest.fn(); | |
| const TestComponent = () => { | |
| useLayoutEffect(() => { | |
| mockFn(); | |
| }, []); | |
| return <div>Hello</div>; | |
| }; | |
| render(<TestComponent />); | |
| expect(mockFn).toHaveBeenCalledTimes(1); | |
| }); |
Adding some useful hooks to the library for using with
Summary by CodeRabbit
TogglePrimitivecomponent with new event handler properties for better integration with external click and key down handlers.Tablecomponent in the documentation for improved visual presentation.FullHeightScrollcomponent to allow additional styling through a new className prop.TogglePrimitivecomponent to ensure seamless integration with external handlers.FullPageScrollcomponent for improved layout and appearance.Documentationmodule to enhance the visual presentation of theDocsTablecomponent.