Create 'copyToClipboard' utility function #9449
Conversation
WalkthroughThe pull request introduces enhancements to the application's localization and user interface, focusing on improving the phone number copy functionality in the Doctor Video Slideover component. New localization entries are added to Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(2 hunks)src/components/Facility/DoctorVideoSlideover.tsx(4 hunks)
🔇 Additional comments (3)
src/components/Facility/DoctorVideoSlideover.tsx (2)
2-2: LGTM: Clean implementation of required imports and state management
The additions of useTranslation, Notification utility, and copied state are well-structured and follow React best practices.
Also applies to: 16-16, 243-244
317-329: LGTM: Well-structured UI implementation with proper accessibility
The implementation includes:
- Proper tooltip usage for user guidance
- Dynamic icon changes for visual feedback
- Internationalized messages
public/locale/en.json (1)
552-552: LGTM: Clear and consistent translation entries
The added translations for phone number copy functionality are well-formatted and maintain consistency with existing entries.
Also applies to: 1145-1145
| const handleCopy = async ( | ||
| e: React.MouseEvent<HTMLAnchorElement, MouseEvent>, | ||
| ) => { | ||
| e.stopPropagation(); | ||
| await navigator.clipboard.writeText(user?.alt_phone_number || ""); | ||
|
|
||
| Notification.Success({ | ||
| msg: t("phone_number_copied"), | ||
| }); | ||
|
|
||
| setCopied(true); | ||
|
|
||
| setTimeout(() => { | ||
| setCopied(false); | ||
| }, 2000); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for clipboard operations
While the implementation is good, it should handle potential clipboard operation failures.
Consider this improved implementation:
const handleCopy = async (
e: React.MouseEvent<HTMLAnchorElement, MouseEvent>,
) => {
e.stopPropagation();
- await navigator.clipboard.writeText(user?.alt_phone_number || "");
-
- Notification.Success({
- msg: t("phone_number_copied"),
- });
-
- setCopied(true);
-
- setTimeout(() => {
- setCopied(false);
- }, 2000);
+ try {
+ await navigator.clipboard.writeText(user?.alt_phone_number || "");
+
+ Notification.Success({
+ msg: t("phone_number_copied"),
+ });
+
+ setCopied(true);
+
+ setTimeout(() => {
+ setCopied(false);
+ }, 2000);
+ } catch (error) {
+ Notification.Error({
+ msg: "Failed to copy phone number",
+ });
+ }
};📝 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 handleCopy = async ( | |
| e: React.MouseEvent<HTMLAnchorElement, MouseEvent>, | |
| ) => { | |
| e.stopPropagation(); | |
| await navigator.clipboard.writeText(user?.alt_phone_number || ""); | |
| Notification.Success({ | |
| msg: t("phone_number_copied"), | |
| }); | |
| setCopied(true); | |
| setTimeout(() => { | |
| setCopied(false); | |
| }, 2000); | |
| }; | |
| const handleCopy = async ( | |
| e: React.MouseEvent<HTMLAnchorElement, MouseEvent>, | |
| ) => { | |
| e.stopPropagation(); | |
| try { | |
| await navigator.clipboard.writeText(user?.alt_phone_number || ""); | |
| Notification.Success({ | |
| msg: t("phone_number_copied"), | |
| }); | |
| setCopied(true); | |
| setTimeout(() => { | |
| setCopied(false); | |
| }, 2000); | |
| } catch (error) { | |
| Notification.Error({ | |
| msg: "Failed to copy phone number", | |
| }); | |
| } | |
| }; |
| const handleCopy = async ( | ||
| e: React.MouseEvent<HTMLAnchorElement, MouseEvent>, | ||
| ) => { | ||
| e.stopPropagation(); | ||
| await navigator.clipboard.writeText(user?.alt_phone_number || ""); | ||
|
|
||
| Notification.Success({ | ||
| msg: t("phone_number_copied"), | ||
| }); | ||
|
|
||
| setCopied(true); | ||
|
|
||
| setTimeout(() => { | ||
| setCopied(false); | ||
| }, 2000); | ||
| }; |
There was a problem hiding this comment.
can we make a utility function called copyToClipboard in utils.ts and invoke that instead?
There was a problem hiding this comment.
can we make a utility function called
copyToClipboardin utils.ts and invoke that instead?
yeah sure @rithviknishad ,
but can we go with a custom hook instead of util, we also need state of the copy to change icon, incase we don't want to include that extra logic in 'DoctorVideoSlideover'
The same logic(hook) can be reused at other places like 'Warranty Card' etc.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Utils/utils.ts (1)
568-593: Add cleanup for setTimeout on unmountThe implementation looks good, but the setTimeout should be cleaned up when the component unmounts to prevent memory leaks.
export const useClipboard = () => { const [copied, setCopied] = useState(false); + const timeoutRef = useRef<NodeJS.Timeout>(); const copyToClipboard = async ( text: string, successMsg: string, errorMsg: string, iconResetDuration = 2500, ) => { if (!text) { Notification.Error({ msg: "Nothing To Copy" }); return; } try { await navigator.clipboard.writeText(text); setCopied(true); Notification.Success({ msg: successMsg }); - setTimeout(() => setCopied(false), iconResetDuration); + timeoutRef.current = setTimeout(() => setCopied(false), iconResetDuration); } catch (err) { Notification.Error({ msg: errorMsg }); } }; + useEffect(() => { + return () => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + } + }; + }, []); return { copied, copyToClipboard }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(3 hunks)src/Utils/utils.ts(2 hunks)src/components/Facility/DoctorVideoSlideover.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Facility/DoctorVideoSlideover.tsx (1)
243-244: LGTM! Clean implementation of copy functionality
The clipboard integration is well implemented with:
- Proper use of translations for messages
- Visual feedback through icon change
- Error handling via the useClipboard hook
Also applies to: 301-318
public/locale/en.json (1)
552-552: LGTM! Translation strings are complete and consistent
All necessary strings for the copy functionality are properly added:
- copy_phone_number
- nothing_to_copy
- phone_copy_failed
- phone_number_copied
Also applies to: 1044-1044, 1143-1143, 1147-1147
|
@rajku-dev once the PR is ready for testing for review tag me |
src/Utils/utils.ts
Outdated
| export const useClipboard = () => { | ||
| const [copied, setCopied] = useState(false); | ||
|
|
||
| const copyToClipboard = async ( | ||
| text: string, | ||
| successMsg: string, | ||
| errorMsg: string, | ||
| iconResetDuration = 2500, | ||
| ) => { | ||
| if (!text) { | ||
| Notification.Error({ msg: "Nothing To Copy" }); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await navigator.clipboard.writeText(text); | ||
| setCopied(true); | ||
| Notification.Success({ msg: successMsg }); | ||
| setTimeout(() => setCopied(false), iconResetDuration); | ||
| } catch (err) { | ||
| Notification.Error({ msg: errorMsg }); | ||
| } | ||
| }; | ||
|
|
||
| return { copied, copyToClipboard }; | ||
| }; |
There was a problem hiding this comment.
i don't see any reason why this needs to be a hook. Let's keep it as a simple fn. instead
There was a problem hiding this comment.
i don't see any reason why this needs to be a hook. Let's keep it as a simple fn. instead
I figured that out, made the required changes @rithviknishad
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Utils/utils.ts (3)
566-573: Add parameter validation and improve TypeScript typesThe function parameters could benefit from better type safety and validation.
Consider these improvements:
export const copyToClipboard = async ( text: string, successMsg: string, errorMsg: string, onSuccess: () => void, - onError: () => void, + onResetIcon: () => void, - resetIconDuration = 2500, + resetIconDuration: number = 2500, ) => { + if (resetIconDuration < 0) { + throw new Error('resetIconDuration must be a positive number'); + }
579-586: Improve error handling specificityThe catch block could provide more specific error handling based on the type of error encountered.
Consider this enhancement:
try { await navigator.clipboard.writeText(text); Notification.Success({ msg: successMsg }); onSuccess(); setTimeout(() => onResetIcon(), resetIconDuration); } catch (err) { - Notification.Error({ msg: errorMsg }); + if (err instanceof Error) { + Notification.Error({ + msg: `${errorMsg}: ${err.message}` + }); + } else { + Notification.Error({ msg: errorMsg }); + } }
566-587: Consider separating UI state management from clipboard operationsThe function currently handles both clipboard operations and UI state management, which violates the Single Responsibility Principle.
Consider splitting this into two functions:
- A pure clipboard utility function that handles only the clipboard operation
- A higher-level function in the UI component that handles the UI state management
Example approach:
// In utils.ts export const copyToClipboard = async (text: string): Promise<void> => { if (!text) { throw new Error("Nothing to copy"); } return navigator.clipboard.writeText(text); }; // In your UI component const handleCopy = async () => { try { await copyToClipboard(text); Notification.Success({ msg: successMsg }); onSuccess(); setTimeout(() => onResetIcon(), resetIconDuration); } catch (err) { Notification.Error({ msg: errorMsg }); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Utils/utils.ts(2 hunks)src/components/Facility/DoctorVideoSlideover.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Facility/DoctorVideoSlideover.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Utils/utils.ts(2 hunks)
🔇 Additional comments (1)
src/Utils/utils.ts (1)
6-6: LGTM!
The Notification module import is correctly placed and properly scoped.
src/Utils/utils.ts
Outdated
| if (!text) { | ||
| Notification.Error({ msg: "Nothing To Copy" }); | ||
| return; |
There was a problem hiding this comment.
Use the provided errorMsg parameter for consistency
The error message is hardcoded as "Nothing To Copy" instead of using the provided errorMsg parameter. This creates inconsistency in error handling and breaks the internationalization support.
Apply this diff to fix the issue:
if (!text) {
- Notification.Error({ msg: "Nothing To Copy" });
+ Notification.Error({ msg: errorMsg });
return;
}📝 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) { | |
| Notification.Error({ msg: "Nothing To Copy" }); | |
| return; | |
| if (!text) { | |
| Notification.Error({ msg: errorMsg }); | |
| return; |
src/Utils/utils.ts
Outdated
| export const copyToClipboard = async ( | ||
| text: string, | ||
| successMsg: string, | ||
| errorMsg: string, | ||
| onSuccess: () => void, | ||
| onResetIcon: () => void, | ||
| resetIconDuration = 2500, | ||
| ) => { | ||
| if (!text) { | ||
| Notification.Error({ msg: "Nothing To Copy" }); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await navigator.clipboard.writeText(text); | ||
| Notification.Success({ msg: successMsg }); | ||
| onSuccess(); | ||
| setTimeout(() => onResetIcon(), resetIconDuration); | ||
| } catch (err) { | ||
| Notification.Error({ msg: errorMsg }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve function signature and documentation
The function would benefit from a more maintainable signature using an options object pattern and proper documentation.
Consider this refactoring:
-export const copyToClipboard = async (
- text: string,
- successMsg: string,
- errorMsg: string,
- onSuccess: () => void,
- onResetIcon: () => void,
- resetIconDuration = 2500,
-) => {
+/**
+ * Copies text to clipboard with visual feedback.
+ *
+ * @param options - Configuration options
+ * @param options.text - Text to copy to clipboard
+ * @param options.successMsg - Message to show on successful copy
+ * @param options.errorMsg - Message to show on copy failure or empty text
+ * @param options.onSuccess - Callback function called after successful copy
+ * @param options.onResetIcon - Callback function to reset icon state
+ * @param options.resetIconDuration - Duration in ms before resetting icon (default: 2500)
+ * @returns Promise that resolves when the copy operation is complete
+ */
+export const copyToClipboard = async ({
+ text,
+ successMsg,
+ errorMsg,
+ onSuccess,
+ onResetIcon,
+ resetIconDuration = 2500,
+}: {
+ text: string;
+ successMsg: string;
+ errorMsg: string;
+ onSuccess: () => void;
+ onResetIcon: () => void;
+ resetIconDuration?: number;
+}) => {
if (!text) {
Notification.Error({ msg: errorMsg });
return;
}
try {
await navigator.clipboard.writeText(text);
Notification.Success({ msg: successMsg });
onSuccess();
setTimeout(() => onResetIcon(), resetIconDuration);
} catch (err) {
Notification.Error({ msg: errorMsg });
}
};This refactoring:
- Uses an options object for better maintainability
- Adds comprehensive JSDoc documentation
- Makes the resetIconDuration parameter explicitly optional in the type system
📝 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 const copyToClipboard = async ( | |
| text: string, | |
| successMsg: string, | |
| errorMsg: string, | |
| onSuccess: () => void, | |
| onResetIcon: () => void, | |
| resetIconDuration = 2500, | |
| ) => { | |
| if (!text) { | |
| Notification.Error({ msg: "Nothing To Copy" }); | |
| return; | |
| } | |
| try { | |
| await navigator.clipboard.writeText(text); | |
| Notification.Success({ msg: successMsg }); | |
| onSuccess(); | |
| setTimeout(() => onResetIcon(), resetIconDuration); | |
| } catch (err) { | |
| Notification.Error({ msg: errorMsg }); | |
| } | |
| }; | |
| /** | |
| * Copies text to clipboard with visual feedback. | |
| * | |
| * @param options - Configuration options | |
| * @param options.text - Text to copy to clipboard | |
| * @param options.successMsg - Message to show on successful copy | |
| * @param options.errorMsg - Message to show on copy failure or empty text | |
| * @param options.onSuccess - Callback function called after successful copy | |
| * @param options.onResetIcon - Callback function to reset icon state | |
| * @param options.resetIconDuration - Duration in ms before resetting icon (default: 2500) | |
| * @returns Promise that resolves when the copy operation is complete | |
| */ | |
| export const copyToClipboard = async ({ | |
| text, | |
| successMsg, | |
| errorMsg, | |
| onSuccess, | |
| onResetIcon, | |
| resetIconDuration = 2500, | |
| }: { | |
| text: string; | |
| successMsg: string; | |
| errorMsg: string; | |
| onSuccess: () => void; | |
| onResetIcon: () => void; | |
| resetIconDuration?: number; | |
| }) => { | |
| if (!text) { | |
| Notification.Error({ msg: errorMsg }); | |
| return; | |
| } | |
| try { | |
| await navigator.clipboard.writeText(text); | |
| Notification.Success({ msg: successMsg }); | |
| onSuccess(); | |
| setTimeout(() => onResetIcon(), resetIconDuration); | |
| } catch (err) { | |
| Notification.Error({ msg: errorMsg }); | |
| } | |
| }; |
src/Utils/utils.ts
Outdated
| export const copyToClipboard = async ( | ||
| text: string, | ||
| successMsg: string, | ||
| errorMsg: string, | ||
| onSuccess: () => void, | ||
| onResetIcon: () => void, | ||
| resetIconDuration = 2500, | ||
| ) => { | ||
| if (!text) { | ||
| Notification.Error({ msg: "Nothing To Copy" }); | ||
| return; |
There was a problem hiding this comment.
let's not show this icon and call this function in the first place if there's nothing to copy.
src/Utils/utils.ts
Outdated
| export const copyToClipboard = async ( | ||
| text: string, | ||
| successMsg: string, | ||
| errorMsg: string, | ||
| onSuccess: () => void, | ||
| onResetIcon: () => void, | ||
| resetIconDuration = 2500, | ||
| ) => { |
There was a problem hiding this comment.
make the function do only what it's meant to do. keep it simple. it is well known that writeText throws only if environment doesn't allow it.
Ref: https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText#exceptions
export const copyToClipboard = async (content: string) => {
try {
await navigator.clipboard.writeText(text);
} catch (err) {
Notification.Error({ msg: "Copying is not allowed" });
}
}There was a problem hiding this comment.
sorry for the clutter @rithviknishad, I have made the required changes as requested
…ku-dev/care_fe into issue/9444/copy-notification
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Utils/utils.ts (1)
6-6: Consider using named imports for better maintainabilityInstead of importing the entire Notification module, consider importing only the specific notifications used.
-import * as Notification from "@/Utils/Notifications"; +import { Success as NotificationSuccess, Error as NotificationError } from "@/Utils/Notifications";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/Utils/utils.ts(2 hunks)src/components/Facility/DoctorVideoSlideover.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- public/locale/en.json
- src/components/Facility/DoctorVideoSlideover.tsx
🔇 Additional comments (1)
src/Utils/utils.ts (1)
566-573: Verify integration with phone number copying feature
Since this function is intended for copying phone numbers, let's verify its integration with the DoctorVideoSlideover component.
✅ Verification successful
Let me gather more context about the implementation to ensure proper integration.
Let me check one final aspect to ensure the implementation includes proper error handling and accessibility.
Integration with phone number copying feature is properly implemented
The implementation shows appropriate integration with:
- Correct usage of
copyToClipboardfunction with phone number data - Visual feedback using copied state and icon change
- Tooltip for user guidance
- Proper event handling with stopPropagation
- Timeout-based state reset
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of copyToClipboard in DoctorVideoSlideover component
rg -A 5 "copyToClipboard.*phone" src/
Length of output: 591
Script:
#!/bin/bash
# Check the full context of phone number copying implementation
ast-grep --pattern 'copyToClipboard($$$phone$$$)' src/
# Check for any other phone-related copy implementations
rg -B 5 -A 5 "phone.*copy|copy.*phone" src/
# Look for the state management of copied
ast-grep --pattern 'const [copied, setCopied] = useState' src/
Length of output: 1750
Script:
#!/bin/bash
# Check for the complete DoctorVideoSlideover component implementation
rg -B 10 -A 10 "const DoctorVideoSlideover" src/
# Check for accessibility attributes and error handling
rg -A 5 "aria-label.*phone|role.*button.*phone" src/
Length of output: 591
Jacobjeevan
added
reviewed
rajku-dev
changed the title
|
@rajku-dev Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
Recording.2024-12-16.115145.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation