Create reusable CopyButton component
#9498
Conversation
WalkthroughThis pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/Utils/utils.ts (1)
567-570: Consider enhancing the error message with context.While the null check is a good addition, the error message could be more specific about what content was empty.
- Notification.Error({ msg: "Nothing to copy here!" }); + Notification.Error({ msg: "Cannot copy empty content" });src/components/Licenses/SBOMViewer.tsx (1)
Line range hint
7-7: Consider creating a custom hook for copy operations.Multiple components share similar copy status management logic. Consider extracting this into a reusable hook:
// src/hooks/useCopyToClipboard.ts export const useCopyToClipboard = () => { const [isCopied, setIsCopied] = useState(false); useEffect(() => { let timeoutId: NodeJS.Timeout; if (isCopied) { timeoutId = setTimeout(() => setIsCopied(false), 2500); } return () => clearTimeout(timeoutId); }, [isCopied]); const copy = async (content: string) => { await copyToClipboard(content); setIsCopied(true); }; return { isCopied, copy }; };This would simplify the components and ensure consistent behavior across the application.
Also applies to: 11-11
src/components/Shifting/ShiftDetails.tsx (2)
45-55: Consider cleanup and constant extraction improvementsThe button implementation looks good, but there are two suggestions for improvement:
- Clear the timeout when the component unmounts to prevent memory leaks
- Extract the timeout duration as a named constant
Consider applying these changes:
+ const COPY_FEEDBACK_TIMEOUT = 2500; + const showCopyToclipBoard = (data: any) => { + const timeoutRef = useRef<NodeJS.Timeout>(); + + useEffect(() => { + return () => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + } + }; + }, []); + return ( <button className="tooltip tooltip-top" onClick={() => { copyToClipboard(copyContent(data)); setIsCopied(true); - setTimeout(() => setIsCopied(false), 2500); + timeoutRef.current = setTimeout(() => setIsCopied(false), COPY_FEEDBACK_TIMEOUT); }} >
Line range hint
58-82: Add type safety to copyContent functionThe
copyContentfunction usesanytype for its parameter. Consider adding proper typing for better type safety and maintainability.Consider applying these changes:
- const copyContent = (data: any) => { + interface ShiftDetailsData { + patient_object?: { + name?: string; + phone_number?: string; + address?: string; + }; + origin_facility_object?: { + name?: string; + }; + reason?: string; + assigned_facility_type?: string; + } + + const copyContent = (data: ShiftDetailsData) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Utils/utils.ts(1 hunks)src/components/Assets/AssetWarrantyCard.tsx(2 hunks)src/components/Licenses/SBOMViewer.tsx(3 hunks)src/components/Shifting/ShiftDetails.tsx(2 hunks)
🔇 Additional comments (1)
src/components/Shifting/ShiftDetails.tsx (1)
28-28: LGTM: Import statement is correctly placed
The copyToClipboard utility import is properly organized with other utility imports.
Co-authored-by: Rithvik Nishad <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/Common/CopyButton.tsx (1)
9-20: Add JSDoc documentation and improve type constraints.While the interface is well-structured, consider these improvements for better maintainability and type safety:
+/** + * Props for the CopyButton component + * @property {string} content - The text content to be copied + * @property {string} [tooltipContent] - Custom tooltip text + * @property {string} [btnContent] - Custom button text + * @property {number} [resetDuration] - Duration in ms to show copied state + * @property {string} [iconClassName] - CSS class for the icon + * @property {string} [btnClassName] - CSS class for the button + * @property {Object} [icons] - Custom icons for copy states + */ interface CopyButtonProps { content: string; tooltipContent?: string; btnContent?: string; - resetDuration?: number; + resetDuration?: number & { __brand: 'PositiveNumber' }; // Type constraint for positive numbers iconClassName?: string; btnClassName?: string; icons?: { - copied: IconName; - copy: IconName; + copied: Extract<IconName, 'l-check'>; // More specific type + copy: Extract<IconName, 'l-copy'>; }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/CopyButton.tsx(1 hunks)
🔇 Additional comments (1)
src/components/Common/CopyButton.tsx (1)
1-7: LGTM! Imports are well-organized and appropriate.
The imports are logically grouped and all are necessary for the component's functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Assets/AssetWarrantyCard.tsx(2 hunks)src/components/Common/CopyButton.tsx(1 hunks)src/components/Licenses/SBOMViewer.tsx(3 hunks)src/components/Shifting/ShiftDetails.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Shifting/ShiftDetails.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Licenses/SBOMViewer.tsx
[error] 183-183: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🔇 Additional comments (5)
src/components/Common/CopyButton.tsx (4)
12-18: LGTM! Well-structured props interface.
The props interface extends button HTML attributes and variant props, providing good flexibility and type safety.
25-37:
Fix potential memory leak in setTimeout.
The current implementation doesn't clean up the timeout when the component unmounts or when isCopied changes.
Apply this diff to fix the memory leak:
const CopyButton = ({ ... }: CopyButtonProps) => {
const [isCopied, setIsCopied] = useState(false);
+
+ React.useEffect(() => {
+ let timeoutId: NodeJS.Timeout;
+ if (isCopied) {
+ timeoutId = setTimeout(() => setIsCopied(false), 2500);
+ }
+ return () => clearTimeout(timeoutId);
+ }, [isCopied]);
return (
<TooltipProvider>
<TooltipComponent content={isCopied ? "Copied!" : tooltipContent}>
<Button
variant="link"
size={size}
onClick={() => {
copyToClipboard(content);
setIsCopied(true);
- setTimeout(() => setIsCopied(false), 2500);
}}
>Likely invalid or redundant comment.
30-38:
Add error handling for copyToClipboard operation.
The current implementation doesn't handle potential errors from the copyToClipboard utility.
Apply this diff to add error handling:
<Button
variant="link"
size={size}
onClick={() => {
- copyToClipboard(content);
- setIsCopied(true);
+ try {
+ copyToClipboard(content);
+ setIsCopied(true);
+ } catch (error) {
+ console.error('Failed to copy:', error);
+ // Consider showing error state in tooltip
+ }
}}
>Likely invalid or redundant comment.
30-38: 🛠️ Refactor suggestion
Enhance button accessibility.
The button lacks proper accessibility attributes.
Apply this diff to improve accessibility:
<Button
variant="link"
size={size}
+ type="button"
+ disabled={isCopied}
+ aria-label={`Copy ${content} to clipboard`}
onClick={() => {
copyToClipboard(content);
setIsCopied(true);
}}
>Likely invalid or redundant comment.
src/components/Assets/AssetWarrantyCard.tsx (1)
33-39: LGTM! Clean implementation of CopyButton.
The component correctly uses CopyButton for the serial number copy functionality. The implementation is clean and maintains good type safety by checking for the existence of the serial number before rendering the CopyButton.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Licenses/SBOMViewer.tsx (1)
185-185: Use double quotes for consistency.Replace single quotes with double quotes to match the project's style.
- return ''; + return "";🧰 Tools
🪛 GitHub Check: lint
[failure] 185-185:
Replace''with""🪛 eslint
[error] 185-185: Replace
''with""(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Licenses/SBOMViewer.tsx(3 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
src/components/Licenses/SBOMViewer.tsx
[failure] 185-185:
Replace '' with ""
🪛 eslint
src/components/Licenses/SBOMViewer.tsx
[error] 185-185: Replace '' with ""
(prettier/prettier)
🔇 Additional comments (1)
src/components/Licenses/SBOMViewer.tsx (1)
7-7: LGTM!
The import statement follows the project's conventions and is correctly placed with other component imports.
Co-authored-by: Rithvik Nishad <[email protected]>
Co-authored-by: Rithvik Nishad <[email protected]>
Co-authored-by: Rithvik Nishad <[email protected]>
|
@rithviknishad, can we pass the success/error messages to this utility via this component? |
|
i don't understand what error would come up when copying, the only notification that needs to be there is "Copied to clipboard" right? |
that's a point. |
rithviknishad
added
needs review
and removed
question
Co-authored-by: Rithvik Nishad <[email protected]>
made the required changes |
|
@rajku-dev fix the cypress test related to your PR |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cypress/pageobject/Patient/PatientDoctorConnect.ts (1)
25-25: Consider scoping to the parent context.
You're using.get("#copy-alt-phone-button")after selecting the parent element, which restarts the query fromdocument. If your intention is to find the button only within the parent, you might want to use.find("#copy-alt-phone-button")to scope the search and prevent accidentally selecting elements elsewhere on the page.- .get("#copy-alt-phone-button") + .find("#copy-alt-phone-button")src/components/Common/CopyButton.tsx (2)
13-19: Consider makingcontentrequired.
If the component is always expected to copy some non-empty string, removing| undefinedfromcontentand avoiding runtime checks might simplify the API. However, if intentionally optional, returningnullearly is fine.
42-51: Add anaria-labelfor better accessibility.
Providing an explicitaria-labeldescribing the copy action improves accessibility for screen reader users.<Button id={id} variant="link" size={size} + aria-label={t("copy_to_clipboard")} onClick={() => { copyToClipboard(content); setIsCopied(true); setTimeout(() => setIsCopied(false), 2500); }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cypress/pageobject/Patient/PatientDoctorConnect.ts(1 hunks)src/components/Common/CopyButton.tsx(1 hunks)src/components/Facility/DoctorVideoSlideover.tsx(3 hunks)
🔇 Additional comments (4)
src/components/Common/CopyButton.tsx (1)
31-31: Returning null could cause layout shifts.
When content is undefined, the component renders nothing. Ensure that callers expect the button to be absent in that scenario.
src/components/Facility/DoctorVideoSlideover.tsx (3)
8-8: Great integration of CopyButton.
The import of CopyButton centralizes and streamlines clipboard functionality, making the code more consistent across the application.
188-192: Potential PII exposure in shared link.
Appending window.location.href to the WhatsApp message could expose sensitive information if the URL includes patient details. Consider reviewing whether this data should remain private.
300-306: Good usage of CopyButton.
Passing alt_phone_number as content nicely leverages the new component's capability to handle empty or valid content. The optional tooltip also provides a consistent user experience.
|
running individual cypress test (cloud api) in chrome for debugging purposes, it is failing due to a different reason compared to it failing here on github |
@nihal467 |
Jacobjeevan
added
needs testing
and removed
question
Proposed Changes
copyToClipboardutil to handle null copy operationsAssetWarrantyCard,SBOMViewer,ShiftDetailsto usecopyToClipboardutility@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
CopyButtoncomponent for improved clipboard functionality across various components.Bug Fixes
CopyButton.