Add CopyPrimitive components for copying text functionality #566
Conversation
WalkthroughThe changes introduce a new context and several components related to copy functionality within a React application. The Changes
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/components/ui/Avatar/Avatar.tsxOops! Something went wrong! :( ESLint: 8.56.0 Error: Failed to load parser '@babel/eslint-parser' declared in '.eslintrc.cjs': Cannot find module '@babel/core/package.json'
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #566 +/- ##
==========================================
- Coverage 92.30% 92.10% -0.21%
==========================================
Files 12 13 +1
Lines 65 76 +11
Branches 15 16 +1
==========================================
+ Hits 60 70 +10
- Misses 5 6 +1 ☔ View full report in Codecov by Sentry. |
![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 and nitpick comments (4)
src/core/primitives/Copy/fragments/CopyPrimitiveFeedback.tsx (1)
5-13: Remove unused children propThe component accepts a
childrenprop but never uses it in the render method. This could be misleading to developers trying to use the component.Consider removing the
childrenprop from the destructuring since it's not being used:-const CopyPrimitiveFeedback = ({ children, isCopiedText = 'Copied!', ...props }: any) => { +const CopyPrimitiveFeedback = ({ isCopiedText = 'Copied!', ...props }: any) => {src/core/primitives/Copy/fragments/CopyPrimitiveRoot.tsx (2)
1-4: Consider adding proper TypeScript types for importsThe presence of TypeScript in the project suggests we should properly type our imports and dependencies.
Consider adding type definitions:
-import Primitive from '~/core/primitives/Primitive'; +import type { PrimitiveButtonProps } from '~/core/primitives/Primitive'; +import Primitive from '~/core/primitives/Primitive'; -import CopyPrimitiveContext from '../contexts/CopyPrimitiveContext'; +import type { CopyPrimitiveContextValue } from '../contexts/CopyPrimitiveContext'; +import CopyPrimitiveContext from '../contexts/CopyPrimitiveContext';
9-12: Optimize context value with useMemoThe context value object is recreated on every render, which could cause unnecessary rerenders in consuming components.
Consider memoizing the context value:
+import React, { useState, useMemo } from 'react'; - const values = { + const values = useMemo(() => ({ isCopied, setIsCopied - }; + }), [isCopied]);src/core/primitives/Copy/fragments/CopyPrimitiveTrigger.tsx (1)
7-7: Add error handling for missing context.The component might break if used outside of CopyPrimitiveContext provider.
- const { setIsCopied, isCopied } = useContext(CopyPrimitiveContext); + const context = useContext(CopyPrimitiveContext); + if (!context) { + throw new Error('CopyTrigger must be used within a CopyPrimitiveContext.Provider'); + } + const { setIsCopied, isCopied } = context;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/core/primitives/Copy/contexts/CopyPrimitiveContext.tsx(1 hunks)src/core/primitives/Copy/fragments/CopyPrimitiveFeedback.tsx(1 hunks)src/core/primitives/Copy/fragments/CopyPrimitiveRoot.tsx(1 hunks)src/core/primitives/Copy/fragments/CopyPrimitiveTrigger.tsx(1 hunks)src/core/primitives/Copy/index.tsx(1 hunks)src/core/primitives/Copy/stories/CopyPrimitive.stories.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/core/primitives/Copy/contexts/CopyPrimitiveContext.tsx
- src/core/primitives/Copy/stories/CopyPrimitive.stories.tsx
🔇 Additional comments (5)
src/core/primitives/Copy/index.tsx (2)
11-11: LGTM: Clean default export
The default export is appropriate for this compound component.
5-9: Well-structured compound component implementation
The compound component pattern is well implemented here, providing a clean API for the Copy Primitive feature. The use of as const ensures proper type inference and immutability.
Let's verify the component structure and usage:
✅ Verification successful
Well-implemented compound component pattern confirmed
The implementation shows a proper compound component structure with:
- A Root component managing shared state through React Context
- A Trigger component handling copy functionality with clipboard API
- A Feedback component for copy status indication
- Proper context sharing between components
- Clean separation of concerns
The component structure follows React best practices and the as const assertion in the original code ensures type safety for the compound component API.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the component structure and potential usage patterns
# Check if the fragment files exist
echo "Verifying fragment files existence:"
fd "CopyPrimitive(Root|Trigger|Feedback).tsx" src/core/primitives/Copy/fragments
# Look for any existing usage patterns
echo "\nChecking usage patterns:"
rg -A 2 "CopyPrimitive\." --type tsx
Length of output: 471
Script:
#!/bin/bash
# Let's verify the usage patterns with the correct file extension
echo "Checking usage patterns:"
rg -A 2 "CopyPrimitive\." --type ts --type tsx
# Let's also check the implementation of each component to verify proper compound structure
echo "\nChecking CopyPrimitiveRoot implementation:"
cat src/core/primitives/Copy/fragments/CopyPrimitiveRoot.tsx
echo "\nChecking CopyPrimitiveTrigger implementation:"
cat src/core/primitives/Copy/fragments/CopyPrimitiveTrigger.tsx
echo "\nChecking CopyPrimitiveFeedback implementation:"
cat src/core/primitives/Copy/fragments/CopyPrimitiveFeedback.tsx
Length of output: 2521
src/core/primitives/Copy/fragments/CopyPrimitiveFeedback.tsx (1)
1-3: Verify CopyPrimitiveContext implementation
Let's verify that the context being imported provides the expected shape and types.
src/core/primitives/Copy/fragments/CopyPrimitiveTrigger.tsx (2)
1-3: LGTM! Imports are clean and well-organized.
The necessary dependencies are properly imported, and the relative paths are correctly used for local components.
26-26: Verify component integration and usage.
Let's ensure consistent naming and proper exports across the codebase.
✅ Verification successful
Let me gather more information about the usage of this component across the codebase.
Let me try one more verification with different patterns to check the component's usage:
Component export structure is properly implemented
The verification shows that:
CopyTriggeris correctly imported and exported as part of the compound componentCopyPrimitive.Trigger- The naming is consistent across the files
- The component follows the established pattern of compound components with Root, Trigger, and Feedback subcomponents
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent naming and exports
rg -g '*.tsx' -g '*.ts' "CopyTrigger|CopyPrimitiveTrigger"
# Verify the component is properly imported and used
ast-grep --pattern 'import { $_ } from "~/core/primitives/Copy"'
Length of output: 516
Script:
#!/bin/bash
# Check for Copy component usage patterns
rg -g '*.tsx' -g '*.ts' '<Copy.Trigger' -A 2
# Check if there are any other imports from Copy primitive
ast-grep --pattern 'import { Copy } from "~/core/primitives"'
Length of output: 110
Script:
#!/bin/bash
# Check for Copy component exports and structure
cat src/core/primitives/Copy/index.tsx
# Look for any Copy component usage with different patterns
rg -g '*.tsx' -g '*.ts' 'from.*primitives/Copy'
Length of output: 425
| import CopyPrimitiveRoot from './fragments/CopyPrimitiveRoot'; | ||
| import CopyTrigger from './fragments/CopyPrimitiveTrigger'; | ||
| import CopyFeedback from './fragments/CopyPrimitiveFeedback'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent naming conventions across imports
The import names should match their usage in the CopyPrimitive object for better maintainability and clarity.
Apply this diff to maintain consistency:
-import CopyTrigger from './fragments/CopyPrimitiveTrigger';
-import CopyFeedback from './fragments/CopyPrimitiveFeedback';
+import CopyPrimitiveTrigger from './fragments/CopyPrimitiveTrigger';
+import CopyPrimitiveFeedback from './fragments/CopyPrimitiveFeedback';📝 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 CopyPrimitiveRoot from './fragments/CopyPrimitiveRoot'; | |
| import CopyTrigger from './fragments/CopyPrimitiveTrigger'; | |
| import CopyFeedback from './fragments/CopyPrimitiveFeedback'; | |
| import CopyPrimitiveRoot from './fragments/CopyPrimitiveRoot'; | |
| import CopyPrimitiveTrigger from './fragments/CopyPrimitiveTrigger'; | |
| import CopyPrimitiveFeedback from './fragments/CopyPrimitiveFeedback'; |
| return null; | ||
| } | ||
|
|
||
| return <Primitive.span {...props}>{isCopiedText}</Primitive.span>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for copy feedback
The feedback message should be announced to screen readers when the copy action is successful. Consider adding ARIA attributes for better accessibility.
- return <Primitive.span {...props}>{isCopiedText}</Primitive.span>;
+ return (
+ <Primitive.span
+ role="status"
+ aria-live="polite"
+ {...props}
+ >
+ {isCopiedText}
+ </Primitive.span>
+ );📝 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.
| return <Primitive.span {...props}>{isCopiedText}</Primitive.span>; | |
| return ( | |
| <Primitive.span | |
| role="status" | |
| aria-live="polite" | |
| {...props} | |
| > | |
| {isCopiedText} | |
| </Primitive.span> | |
| ); |
| import Primitive from '~/core/primitives/Primitive'; | ||
| import CopyPrimitiveContext from '../contexts/CopyPrimitiveContext'; | ||
|
|
||
| const CopyPrimitiveFeedback = ({ children, isCopiedText = 'Copied!', ...props }: any) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define proper TypeScript interfaces for component props
The component is using the any type which defeats the purpose of TypeScript's type safety. Consider defining a proper interface for the props.
+interface CopyPrimitiveFeedbackProps extends React.HTMLAttributes<HTMLSpanElement> {
+ isCopiedText?: string;
+ children?: never; // since children prop is not used
+}
-const CopyPrimitiveFeedback = ({ children, isCopiedText = 'Copied!', ...props }: any) => {
+const CopyPrimitiveFeedback = ({ isCopiedText = 'Copied!', ...props }: CopyPrimitiveFeedbackProps) => {📝 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 CopyPrimitiveFeedback = ({ children, isCopiedText = 'Copied!', ...props }: any) => { | |
| interface CopyPrimitiveFeedbackProps extends React.HTMLAttributes<HTMLSpanElement> { | |
| isCopiedText?: string; | |
| children?: never; // since children prop is not used | |
| } | |
| const CopyPrimitiveFeedback = ({ isCopiedText = 'Copied!', ...props }: CopyPrimitiveFeedbackProps) => { |
| const CopyPrimitiveRoot = ({ children, ...props }: any) => { | ||
| const [isCopied, setIsCopied] = useState(false); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper TypeScript types for component props and state
The component uses any type which bypasses TypeScript's type checking benefits. Additionally, the state management could be more explicit.
Consider this improvement:
-const CopyPrimitiveRoot = ({ children, ...props }: any) => {
+interface CopyPrimitiveRootProps extends PrimitiveButtonProps {
+ children: React.ReactNode;
+}
+
+const CopyPrimitiveRoot = ({ children, ...props }: CopyPrimitiveRootProps) => {
- const [isCopied, setIsCopied] = useState(false);
+ const [isCopied, setIsCopied] = useState<boolean>(false);Committable suggestion skipped: line range outside the PR's diff.
| return ( | ||
| <CopyPrimitiveContext.Provider value={values}> | ||
| <Primitive.button {...props}>{children}</Primitive.button> | ||
| </CopyPrimitiveContext.Provider> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for copy functionality
The button component should have proper ARIA attributes to indicate its purpose and state to assistive technologies.
Consider adding these accessibility improvements:
return (
<CopyPrimitiveContext.Provider value={values}>
- <Primitive.button {...props}>{children}</Primitive.button>
+ <Primitive.button
+ role="button"
+ aria-label="Copy to clipboard"
+ aria-pressed={isCopied}
+ {...props}
+ >
+ {children}
+ </Primitive.button>
</CopyPrimitiveContext.Provider>
);Committable suggestion skipped: line range outside the PR's diff.
| import React, { useState } from 'react'; | ||
| import Primitive from '~/core/primitives/Primitive'; | ||
|
|
||
| import CopyPrimitiveContext from '../contexts/CopyPrimitiveContext'; | ||
|
|
||
| const CopyPrimitiveRoot = ({ children, ...props }: any) => { | ||
| const [isCopied, setIsCopied] = useState(false); | ||
|
|
||
| const values = { | ||
| isCopied, | ||
| setIsCopied | ||
| }; | ||
|
|
||
| return ( | ||
| <CopyPrimitiveContext.Provider value={values}> | ||
| <Primitive.button {...props}>{children}</Primitive.button> | ||
| </CopyPrimitiveContext.Provider> | ||
| ); | ||
| }; | ||
|
|
||
| export default CopyPrimitiveRoot; |
There was a problem hiding this comment.
💡 Codebase verification
Add error handling for clipboard operations
The clipboard implementation lacks error handling for potential failures in navigator.clipboard.writeText(). This could lead to unhandled promise rejections since:
- The clipboard API returns a Promise that's not being handled
- No try-catch block surrounds the clipboard operation
- No error state is maintained in the context
Key locations requiring attention:
src/core/primitives/Copy/fragments/CopyPrimitiveTrigger.tsx: Add async/await with try-catch for clipboard operationsrc/core/primitives/Copy/contexts/CopyPrimitiveContext.tsx: Consider adding error state to the context
🔗 Analysis chain
Verify clipboard API implementation
The component seems to be part of a copy functionality implementation, but the actual clipboard interaction isn't visible in this file.
Let's verify the clipboard implementation in related files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for clipboard API usage in related files
echo "Searching for clipboard API usage..."
rg -t typescript -t javascript "navigator\.clipboard|document\.execCommand.*copy" src/core/primitives/Copy/
# Look for error handling patterns
echo "Checking error handling..."
rg -t typescript -t javascript "try.*catch.*clipboard" src/core/primitives/Copy/
Length of output: 408
Script:
#!/bin/bash
# Search for clipboard API usage with correct file extensions
echo "Searching for clipboard API usage..."
rg "navigator\.clipboard|document\.execCommand.*copy" --type-add 'ts:*.{ts,tsx}' --type=ts src/core/primitives/Copy/
# Look for error handling patterns
echo "Checking error handling..."
rg "try.*catch" --type-add 'ts:*.{ts,tsx}' --type=ts src/core/primitives/Copy/
# List all files in the Copy directory to understand the structure
echo "Listing Copy directory files..."
fd . src/core/primitives/Copy/ -t f
Length of output: 896
Script:
#!/bin/bash
# Let's check the implementation of CopyPrimitiveTrigger to see the full context
echo "Checking CopyPrimitiveTrigger implementation..."
cat src/core/primitives/Copy/fragments/CopyPrimitiveTrigger.tsx
# Also check if there are any error boundaries or error states in the context
echo "Checking CopyPrimitiveContext..."
cat src/core/primitives/Copy/contexts/CopyPrimitiveContext.tsx
Length of output: 1264
| import CopyPrimitiveContext from '../contexts/CopyPrimitiveContext'; | ||
|
|
||
| // The triggering action (button) is logically part of the copying mechanism | ||
| const CopyTrigger = ({ children, text = '', resetDelay = 2000, ...props }: any) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define proper TypeScript interface for component props.
Using any type reduces type safety. Consider defining a proper interface:
+interface CopyTriggerProps extends React.ButtonHTMLAttributes<HTMLButtonElement> {
+ children: React.ReactNode;
+ text?: string;
+ resetDelay?: number;
+}
-const CopyTrigger = ({ children, text = '', resetDelay = 2000, ...props }: any) => {
+const CopyTrigger = ({ children, text = '', resetDelay = 2000, ...props }: CopyTriggerProps) => {📝 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 CopyTrigger = ({ children, text = '', resetDelay = 2000, ...props }: any) => { | |
| interface CopyTriggerProps extends React.ButtonHTMLAttributes<HTMLButtonElement> { | |
| children: React.ReactNode; | |
| text?: string; | |
| resetDelay?: number; | |
| } | |
| const CopyTrigger = ({ children, text = '', resetDelay = 2000, ...props }: CopyTriggerProps) => { |
| if (!text || isCopied) { | ||
| return null; | ||
| } | ||
|
|
||
| return <Primitive.button {...props} onClick={handleClick}>{children}</Primitive.button>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and user feedback.
The button should have proper ARIA attributes and loading state indication.
- return <Primitive.button {...props} onClick={handleClick}>{children}</Primitive.button>;
+ return (
+ <Primitive.button
+ {...props}
+ onClick={handleClick}
+ aria-label="Copy to clipboard"
+ aria-pressed={isCopied}
+ disabled={!text}
+ >
+ {children}
+ </Primitive.button>
+ );📝 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.
| if (!text || isCopied) { | |
| return null; | |
| } | |
| return <Primitive.button {...props} onClick={handleClick}>{children}</Primitive.button>; | |
| if (!text || isCopied) { | |
| return null; | |
| } | |
| return ( | |
| <Primitive.button | |
| {...props} | |
| onClick={handleClick} | |
| aria-label="Copy to clipboard" | |
| aria-pressed={isCopied} | |
| disabled={!text} | |
| > | |
| {children} | |
| </Primitive.button> | |
| ); |
| const handleClick = () => { | ||
| if (text) { | ||
| setIsCopied(true); | ||
| navigator.clipboard.writeText(text); | ||
| setTimeout(() => { | ||
| setIsCopied(false); | ||
| }, resetDelay); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Improve error handling and cleanup for clipboard operations.
The current implementation has several potential issues:
- No error handling for clipboard operations
- Possible memory leak from setTimeout
- No cleanup on component unmount
+ const timeoutRef = React.useRef<NodeJS.Timeout>();
+
const handleClick = () => {
if (text) {
setIsCopied(true);
- navigator.clipboard.writeText(text);
- setTimeout(() => {
- setIsCopied(false);
- }, resetDelay);
+ navigator.clipboard.writeText(text)
+ .catch((error) => {
+ console.error('Failed to copy text:', error);
+ setIsCopied(false);
+ });
+ timeoutRef.current = setTimeout(() => {
+ setIsCopied(false);
+ }, resetDelay);
}
};
+
+ React.useEffect(() => {
+ return () => {
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ }
+ };
+ }, []);📝 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 handleClick = () => { | |
| if (text) { | |
| setIsCopied(true); | |
| navigator.clipboard.writeText(text); | |
| setTimeout(() => { | |
| setIsCopied(false); | |
| }, resetDelay); | |
| } | |
| }; | |
| const timeoutRef = React.useRef<NodeJS.Timeout>(); | |
| const handleClick = () => { | |
| if (text) { | |
| setIsCopied(true); | |
| navigator.clipboard.writeText(text) | |
| .catch((error) => { | |
| console.error('Failed to copy text:', error); | |
| setIsCopied(false); | |
| }); | |
| timeoutRef.current = setTimeout(() => { | |
| setIsCopied(false); | |
| }, resetDelay); | |
| } | |
| }; | |
| React.useEffect(() => { | |
| return () => { | |
| if (timeoutRef.current) { | |
| clearTimeout(timeoutRef.current); | |
| } | |
| }; | |
| }, []); |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (11)
src/components/ui/Copy/Copy.tsx (1)
15-20: Export types and add documentation for subcomponentsConsider adding type exports and documentation for the compound components.
Here's a suggested implementation:
+/** Copy component for clipboard operations */ Copy.displayName = COMPONENT_NAME; + +/** Container component for copy functionality */ Copy.Root = CopyPrimitive.Root; + +/** Button that triggers the copy operation */ Copy.Trigger = CopyPrimitive.Trigger; + +/** Component that displays feedback after copying */ Copy.Feedback = CopyPrimitive.Feedback; +export type { + CopyProps, + CopyRootProps, + CopyTriggerProps, + CopyFeedbackProps, +}; export default Copy;src/components/ui/Avatar/Avatar.tsx (2)
Line range hint
8-14: Fix incorrect type definition for propsThe
propstype is currently defined asRecord<string, any>[](array of records), but it's being spread as an object. This will cause runtime errors.Apply this fix to the type definition:
export type AvatarProps = { customRootClass?: string, fallback?: string, className?: string, src?: string, alt?: string, - props?: Record<string, any>[] + [key: string]: any // or better, extend from React.ComponentPropsWithoutRef<typeof AvatarPrimitive.Root> }
Line range hint
31-34: Consider enhancing compound component patternThe component correctly implements the compound pattern by exposing subcomponents. Since this PR adds copy functionality, consider documenting the integration with
CopyPrimitiveif avatars need to be copyable.Example usage pattern that could be documented:
<Avatar> <CopyPrimitive.Root> <Avatar.Image src="..." /> <CopyPrimitive.Trigger>Copy Avatar URL</CopyPrimitive.Trigger> </CopyPrimitive.Root> </Avatar>src/core/primitives/Copy/fragments/CopyPrimitiveTrigger.tsx (3)
6-6: Standardize prop naming for consistency.The prop name
copyContentdiffers from the suggestedtextprop in the TypeScript interface. Consider using consistent naming across the codebase.-const CopyTrigger = ({ children, className = '', copyContent = '', resetDelay = 2000, ...props }: any) => { +const CopyTrigger = ({ children, className = '', text = '', resetDelay = 2000, ...props }: any) => {
23-23: Improve className handling with a utility.Consider using a className utility like
clsxorclassnamesfor better className management.+import clsx from 'clsx'; - return <Primitive.button className={`${rootClass}-trigger ${className}`} {...props} onClick={handleClick}>{children}</Primitive.button>; + return <Primitive.button className={clsx(`${rootClass}-trigger`, className)} {...props} onClick={handleClick}>{children}</Primitive.button>;
5-5: Add comprehensive component documentation.The current comment is insufficient. Add proper JSDoc documentation including:
- Component description
- Props documentation
- Usage examples
-// The triggering action (button) is logically part of the copying mechanism +/** + * CopyTrigger - A button component that triggers copying text to clipboard + * + * @component + * @example + * ```jsx + * <CopyTrigger text="Text to copy"> + * Click to copy + * </CopyTrigger> + * ``` + * + * @param {Object} props + * @param {React.ReactNode} props.children - Button content + * @param {string} [props.text=''] - Text to copy to clipboard + * @param {string} [props.className=''] - Additional CSS classes + * @param {number} [props.resetDelay=2000] - Delay in ms before resetting copied state + */styles/themes/default.scss (2)
10-10: Maintain alphabetical ordering of importsThe new import breaks the alphabetical sequence. Consider moving it between "code" and "heading" imports to maintain consistent ordering.
@import "./components/card.scss"; -@import "./components/copy.scss"; @import "./components/table.scss"; @import "./components/code.scss"; +@import "./components/copy.scss"; @import "./components/heading.scss";
Line range hint
31-78: Consider cleaning up unused code and empty rulesetsThe file contains:
- Commented-out CSS rules that appear to be unused
- Empty rulesets (e.g.,
.rad-ui-tooltip,.rad-ui-tooltip-reference-element)- Unnecessary empty spaces between sections
Consider removing or documenting these to improve maintainability.
src/components/ui/Copy/stories/Copy.stories.tsx (3)
1-3: Consider using consistent import paths and adding type definitions.The imports mix relative and absolute paths. Consider using consistent import paths throughout the codebase.
-import Copy from '../Copy'; -import SandboxEditor from '~/components/tools/SandboxEditor/SandboxEditor'; +import { Copy } from '~/components/ui/Copy'; +import { SandboxEditor } from '~/components/tools/SandboxEditor';
7-9: Enhance SVG icons accessibility and organization.Consider the following improvements:
- Move icons to a dedicated icons directory
- Add ARIA labels and roles for accessibility
-const CopyIcon = () => <svg width="15" height="15" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg"> +const CopyIcon = () => <svg + width="15" + height="15" + viewBox="0 0 15 15" + fill="none" + xmlns="http://www.w3.org/2000/svg" + role="img" + aria-label="Copy" +>
35-39: Enhance story with more comprehensive examples.The current story only demonstrates basic usage. Consider adding:
- Different states (success, error)
- Various content lengths
- Different trigger styles
- Accessibility examples
export const Default = { args: { copyContent: 'Hello, world!', children: 'Copy' } }; export const WithLongContent = { args: { copyContent: 'This is a very long text that demonstrates how the component handles lengthy content...', children: 'Copy' } }; export const CustomTrigger = { args: { copyContent: 'Custom trigger example', children: <button className="custom-button">Copy me!</button> } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/components/ui/Avatar/Avatar.tsx(1 hunks)src/components/ui/Copy/Copy.tsx(1 hunks)src/components/ui/Copy/stories/Copy.stories.tsx(1 hunks)src/core/primitives/Copy/fragments/CopyPrimitiveFeedback.tsx(1 hunks)src/core/primitives/Copy/fragments/CopyPrimitiveRoot.tsx(1 hunks)src/core/primitives/Copy/fragments/CopyPrimitiveTrigger.tsx(1 hunks)src/core/primitives/Copy/stories/CopyPrimitive.stories.tsx(1 hunks)styles/themes/components/copy.scss(1 hunks)styles/themes/default.scss(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- styles/themes/components/copy.scss
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/primitives/Copy/fragments/CopyPrimitiveFeedback.tsx
- src/core/primitives/Copy/stories/CopyPrimitive.stories.tsx
🔇 Additional comments (9)
src/components/ui/Copy/Copy.tsx (2)
1-20: Verify test coverage and Storybook stories
Please ensure that this component has adequate test coverage and Storybook stories.
#!/bin/bash
# Check for test files
echo "Checking for test files..."
fd -e test.tsx -e spec.tsx "Copy" src/
# Check for story files
echo "Checking for story files..."
fd -e stories.tsx "Copy" src/1-2: Verify the import alias configuration
The import path uses the ~ alias. Let's verify this alias is properly configured in the build system.
✅ Verification successful
The ~ alias is properly configured in the build system
The import path using the ~ alias is correctly configured in the tsconfig.json file where it maps ~/* to src/*. This is a valid and working configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for tsconfig.json or other config files that might define path aliases
fd -e json -e js "tsconfig|webpack.config|vite.config" -x cat {} \; | grep -A 5 "paths\|alias"
Length of output: 303
Script:
#!/bin/bash
# Let's check for tsconfig.json files first
fd tsconfig.json -x cat {}
# Also check for any vite config files
fd "vite.config" -x cat {}
Length of output: 1389
src/core/primitives/Copy/fragments/CopyPrimitiveRoot.tsx (4)
1-6: LGTM! Imports are clean and well-organized.
8-8:
Previous typing suggestion remains unaddressed
The any type is still being used, which was highlighted in a previous review. Additionally, spreading unknown props could lead to prop pollution.
Consider this improvement:
-const CopyPrimitiveRoot = ({ customRootClass = '', className = '', children, ...props }: any) => {
+interface CopyPrimitiveRootProps {
+ customRootClass?: string;
+ className?: string;
+ children: React.ReactNode;
+ // Only allow valid HTML span attributes
+ [key: string]: unknown;
+}
+
+const CopyPrimitiveRoot = ({
+ customRootClass = '',
+ className = '',
+ children,
+ ...props
+}: CopyPrimitiveRootProps) => {9-16:
Add error handling state
The context should include error handling for clipboard operations as mentioned in the previous review.
Consider this improvement:
-const [isCopied, setIsCopied] = useState(false);
+const [isCopied, setIsCopied] = useState<boolean>(false);
+const [error, setError] = useState<Error | null>(null);
const rootClass = customClassSwitcher(customRootClass, 'Copy');
const values = {
isCopied,
setIsCopied,
+ error,
+ setError,
rootClass
};1-25: Verify component integration
Let's verify how this component is integrated with other copy primitive components.
✅ Verification successful
Let me gather more information about the component integration by examining the story files and searching for imports with a different approach.
Let me check the content of the related Copy primitive components to understand the complete integration.
Let me check the correct paths for the context and index files.
Based on the gathered information, I can now provide the final response:
Component integration is properly implemented ✓
The CopyPrimitiveRoot component is correctly integrated within a compound component pattern:
- Properly exports context through CopyPrimitiveContext
- Correctly composed with CopyTrigger and CopyFeedback components
- Successfully demonstrated in both Copy and CopyPrimitive stories
- Follows React best practices for compound components
The implementation shows proper integration with:
- Context sharing between components
- Compound component structure in index.tsx
- Working examples in storybook files
- Consistent class naming convention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check component integration and usage patterns
# Check for imports of CopyPrimitiveRoot
echo "Checking usage of CopyPrimitiveRoot..."
rg "import.*CopyPrimitiveRoot" --type=typescript
# Check story files for usage examples
echo "Checking story files..."
fd "Copy.*\.(story|stories)\.(tsx|ts)" src/
# Check test coverage
echo "Checking test files..."
fd "Copy.*\.(test|spec)\.(tsx|ts)" src/
Length of output: 479
Script:
#!/bin/bash
# Check content of story files to understand usage
echo "=== Content of Copy.stories.tsx ==="
cat "src/components/ui/Copy/stories/Copy.stories.tsx"
echo -e "\n=== Content of CopyPrimitive.stories.tsx ==="
cat "src/core/primitives/Copy/stories/CopyPrimitive.stories.tsx"
# Check imports using ripgrep with different file extensions
echo -e "\n=== Checking imports in TypeScript/React files ==="
rg "import.*CopyPrimitiveRoot" -t tsx -t ts -t jsx -t js
# Check for any Copy primitive related components
echo -e "\n=== Checking Copy primitive related files ==="
fd "Copy.*\.(tsx|ts|jsx|js)" src/core/primitives/Copy/
Length of output: 4399
Script:
#!/bin/bash
# Check the implementation of related components
echo "=== Content of CopyPrimitiveTrigger.tsx ==="
cat "src/core/primitives/Copy/fragments/CopyPrimitiveTrigger.tsx"
echo -e "\n=== Content of CopyPrimitiveFeedback.tsx ==="
cat "src/core/primitives/Copy/fragments/CopyPrimitiveFeedback.tsx"
echo -e "\n=== Content of CopyPrimitiveContext.tsx ==="
cat "src/core/primitives/Copy/fragments/CopyPrimitiveContext.tsx"
# Check the index file to see how components are exported
echo -e "\n=== Content of index file ==="
cat "src/core/primitives/Copy/index.ts"
Length of output: 2232
Script:
#!/bin/bash
# Check the context file in the correct location
echo "=== Content of CopyPrimitiveContext.tsx ==="
cat "src/core/primitives/Copy/contexts/CopyPrimitiveContext.tsx"
# Find and check the index file
echo -e "\n=== Looking for index files ==="
fd "index\.(ts|tsx)" src/core/primitives/Copy/
echo -e "\n=== Content of index file if found ==="
cat "src/core/primitives/Copy/index.tsx" 2>/dev/null || cat "src/core/primitives/Copy/index.ts" 2>/dev/null
Length of output: 919
src/components/ui/Avatar/Avatar.tsx (1)
Line range hint 19-27: Consider removing duplicate src prop
The src prop is being passed to both AvatarPrimitive.Root and AvatarPrimitive.Image. This could lead to confusion about which one takes precedence.
Let's check how src is used in the primitive:
Consider passing src only to AvatarPrimitive.Image unless there's a specific need for it in Root:
- <AvatarPrimitive.Root src={src} customRootClass={customRootClass} {...props}>
+ <AvatarPrimitive.Root customRootClass={customRootClass} {...props}>src/core/primitives/Copy/fragments/CopyPrimitiveTrigger.tsx (1)
7-7: LGTM! Context usage is clean and efficient.
src/components/ui/Copy/stories/Copy.stories.tsx (1)
2-2: Verify Copy component implementation and accessibility.
Let's ensure the Copy component is properly implemented and follows accessibility best practices.
| return <CopyPrimitive.Root asChild customRootClass={customRootClass} {...props}> | ||
| <CopyPrimitive.Trigger resetDelay={resetDelay} copyContent={copyContent} >{children}</CopyPrimitive.Trigger> | ||
| <CopyPrimitive.Feedback > | ||
| {isCopiedContent} | ||
| </CopyPrimitive.Feedback> | ||
| </CopyPrimitive.Root>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance component implementation with error handling and accessibility
The current implementation could benefit from several improvements:
- Error handling for clipboard operations
- Accessibility attributes
- Loading and error states
Here's a suggested implementation:
- return <CopyPrimitive.Root asChild customRootClass={customRootClass} {...props}>
- <CopyPrimitive.Trigger resetDelay={resetDelay} copyContent={copyContent} >{children}</CopyPrimitive.Trigger>
- <CopyPrimitive.Feedback >
- {isCopiedContent}
- </CopyPrimitive.Feedback>
- </CopyPrimitive.Root>;
+ return (
+ <CopyPrimitive.Root
+ asChild
+ customRootClass={customRootClass}
+ {...props}
+ >
+ <CopyPrimitive.Trigger
+ resetDelay={resetDelay}
+ copyContent={copyContent}
+ aria-label="Copy to clipboard"
+ onError={(error) => console.error('Failed to copy:', error)}
+ >
+ {children}
+ </CopyPrimitive.Trigger>
+ <CopyPrimitive.Feedback
+ role="status"
+ aria-live="polite"
+ >
+ {isCopiedContent}
+ </CopyPrimitive.Feedback>
+ </CopyPrimitive.Root>
+ );📝 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.
| return <CopyPrimitive.Root asChild customRootClass={customRootClass} {...props}> | |
| <CopyPrimitive.Trigger resetDelay={resetDelay} copyContent={copyContent} >{children}</CopyPrimitive.Trigger> | |
| <CopyPrimitive.Feedback > | |
| {isCopiedContent} | |
| </CopyPrimitive.Feedback> | |
| </CopyPrimitive.Root>; | |
| return ( | |
| <CopyPrimitive.Root | |
| asChild | |
| customRootClass={customRootClass} | |
| {...props} | |
| > | |
| <CopyPrimitive.Trigger | |
| resetDelay={resetDelay} | |
| copyContent={copyContent} | |
| aria-label="Copy to clipboard" | |
| onError={(error) => console.error('Failed to copy:', error)} | |
| > | |
| {children} | |
| </CopyPrimitive.Trigger> | |
| <CopyPrimitive.Feedback | |
| role="status" | |
| aria-live="polite" | |
| > | |
| {isCopiedContent} | |
| </CopyPrimitive.Feedback> | |
| </CopyPrimitive.Root> | |
| ); |
| const Copy = ({ customRootClass = '', children, isCopiedContent = 'Copied!', resetDelay = 2000, copyContent = '', ...props }: any) => { | ||
| return <CopyPrimitive.Root asChild customRootClass={customRootClass} {...props}> | ||
| <CopyPrimitive.Trigger resetDelay={resetDelay} copyContent={copyContent} >{children}</CopyPrimitive.Trigger> | ||
| <CopyPrimitive.Feedback > | ||
| {isCopiedContent} | ||
| </CopyPrimitive.Feedback> | ||
| </CopyPrimitive.Root>; | ||
| }; |
There was a problem hiding this comment.
Define proper TypeScript interfaces for props
The component uses any type which defeats TypeScript's type safety. Consider defining a proper interface for the props.
Here's a suggested implementation:
+interface CopyProps {
+ /** Custom class name for the root element */
+ customRootClass?: string;
+ /** Content to be displayed in the trigger */
+ children: React.ReactNode;
+ /** Text shown when content is copied */
+ isCopiedContent?: string;
+ /** Delay in milliseconds before resetting the copied state */
+ resetDelay?: number;
+ /** Content to be copied to clipboard */
+ copyContent: string;
+}
+
-const Copy = ({ customRootClass = '', children, isCopiedContent = 'Copied!', resetDelay = 2000, copyContent = '', ...props }: any) => {
+const Copy = ({
+ customRootClass = '',
+ children,
+ isCopiedContent = 'Copied!',
+ resetDelay = 2000,
+ copyContent,
+ ...props
+}: CopyProps) => {📝 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 Copy = ({ customRootClass = '', children, isCopiedContent = 'Copied!', resetDelay = 2000, copyContent = '', ...props }: any) => { | |
| return <CopyPrimitive.Root asChild customRootClass={customRootClass} {...props}> | |
| <CopyPrimitive.Trigger resetDelay={resetDelay} copyContent={copyContent} >{children}</CopyPrimitive.Trigger> | |
| <CopyPrimitive.Feedback > | |
| {isCopiedContent} | |
| </CopyPrimitive.Feedback> | |
| </CopyPrimitive.Root>; | |
| }; | |
| interface CopyProps { | |
| /** Custom class name for the root element */ | |
| customRootClass?: string; | |
| /** Content to be displayed in the trigger */ | |
| children: React.ReactNode; | |
| /** Text shown when content is copied */ | |
| isCopiedContent?: string; | |
| /** Delay in milliseconds before resetting the copied state */ | |
| resetDelay?: number; | |
| /** Content to be copied to clipboard */ | |
| copyContent: string; | |
| } | |
| const Copy = ({ | |
| customRootClass = '', | |
| children, | |
| isCopiedContent = 'Copied!', | |
| resetDelay = 2000, | |
| copyContent, | |
| ...props | |
| }: CopyProps) => { | |
| return <CopyPrimitive.Root asChild customRootClass={customRootClass} {...props}> | |
| <CopyPrimitive.Trigger resetDelay={resetDelay} copyContent={copyContent} >{children}</CopyPrimitive.Trigger> | |
| <CopyPrimitive.Feedback > | |
| {isCopiedContent} | |
| </CopyPrimitive.Feedback> | |
| </CopyPrimitive.Root>; | |
| }; |
| return ( | ||
| <CopyPrimitiveContext.Provider value={values}> | ||
| <Primitive.span className={`${rootClass} ${className}`} {...props}>{children}</Primitive.span> | ||
| </CopyPrimitiveContext.Provider> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use semantic HTML and improve accessibility
The component uses a span element which might not be the most semantic choice for copy functionality. Consider using a more appropriate element and adding accessibility attributes.
Consider this improvement:
return (
<CopyPrimitiveContext.Provider value={values}>
- <Primitive.span className={`${rootClass} ${className}`} {...props}>{children}</Primitive.span>
+ <Primitive.div
+ role="region"
+ aria-label="Copyable content"
+ className={`${rootClass}`.trim() + (className ? ` ${className}` : '')}
+ {...props}
+ >
+ {children}
+ </Primitive.div>
</CopyPrimitiveContext.Provider>
);📝 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.
| return ( | |
| <CopyPrimitiveContext.Provider value={values}> | |
| <Primitive.span className={`${rootClass} ${className}`} {...props}>{children}</Primitive.span> | |
| </CopyPrimitiveContext.Provider> | |
| ); | |
| return ( | |
| <CopyPrimitiveContext.Provider value={values}> | |
| <Primitive.div | |
| role="region" | |
| aria-label="Copyable content" | |
| className={`${rootClass}`.trim() + (className ? ` ${className}` : '')} | |
| {...props} | |
| > | |
| {children} | |
| </Primitive.div> | |
| </CopyPrimitiveContext.Provider> | |
| ); |
| if (!copyContent || isCopied) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider UX for button visibility.
Hiding the button after copying might confuse users or prevent them from copying the same content multiple times. Consider:
- Keeping the button visible and updating its text/icon to indicate the copied state
- Allowing users to copy content multiple times without waiting for the reset
- if (!copyContent || isCopied) {
+ if (!copyContent) {
return null;
}📝 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.
| if (!copyContent || isCopied) { | |
| return null; | |
| } | |
| if (!copyContent) { | |
| return null; | |
| } |
| render: (args: any) => <SandboxEditor> | ||
| <div > | ||
| <Copy.Root {...args}> | ||
| <Copy.Trigger copyContent="Hello, world!" {...args}> | ||
| <div className="flex items-center gap-2"> | ||
| <span>Copy</span> | ||
| <CopyIcon /> | ||
| </div> | ||
| </Copy.Trigger> | ||
| <Copy.Feedback {...args}> | ||
| <div className="flex items-center gap-2"> | ||
| <span>Copied!</span> | ||
| <TickIcon /> | ||
| </div> | ||
| </Copy.Feedback> | ||
| </Copy.Root> | ||
| </div> | ||
| </SandboxEditor> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and component flexibility.
Several improvements could enhance this story:
- Replace
anywith proper type definitions - Make copy content configurable via args
- Consider using design system tokens for spacing instead of utility classes
- render: (args: any) => <SandboxEditor>
+ render: (args: CopyProps) => <SandboxEditor>
<div >
<Copy.Root {...args}>
- <Copy.Trigger copyContent="Hello, world!" {...args}>
+ <Copy.Trigger copyContent={args.copyContent} {...args}>
<div className="flex items-center gap-2">Also, consider extracting the hardcoded styles into your design system's spacing tokens:
- <div className="flex items-center gap-2">
+ <div className={styles.container}>Committable suggestion skipped: line range outside the PR's diff.
This PR adds Copy Primitive
Summary by CodeRabbit
Release Notes
New Features
CopyPrimitivemodule, including components for managing copy operations.CopyPrimitiveRoot,CopyPrimitiveTrigger, andCopyPrimitiveFeedbackfor enhanced clipboard interaction.Copycomponent created to simplify the use of copy functionality with customizable options.Documentation
CopyPrimitiveandCopycomponents to demonstrate usage and functionality.Styles
Copycomponent, enhancing its visual presentation and interactivity.