🎨 Palette: Add copy feedback to wallet addresses#152
🎨 Palette: Add copy feedback to wallet addresses#152Dexploarer wants to merge 1 commit intodevelopfrom
Conversation
- Created `CopyButton` component in `apps/app/src/components/shared/CopyButton.tsx` with visual feedback (checkmark and "copied" text). - Updated `Header.tsx` to use `CopyButton` for EVM and Solana addresses. - Updated `tsconfig.json` to include `bun-types` for better local testing compatibility.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience by standardizing the copy-to-clipboard functionality across the application. By introducing a dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| // @ts-expect-error - test uses a narrowed subset of the full app context type. | ||
| vi.spyOn(AppContext, "useApp").mockReturnValue(mockUseApp); |
There was a problem hiding this comment.
TypeScript type assertion bypass (@ts-expect-error)
Using @ts-expect-error to bypass type checking for the mocked context (line 46) can lead to brittle tests and maintenance issues if the context shape changes. It is preferable to define a proper mock type or use partial typing to ensure type safety and reduce the risk of runtime errors.
Recommended solution:
Define a mock type for the context or use Partial<AppContextType> to avoid bypassing TypeScript checks:
const mockUseApp: Partial<AppContextType> = { ... };| const copyButtons = root.findAll((node: ReactTestInstance) => { | ||
| return ( | ||
| node.type === "button" && | ||
| node.props["aria-label"] && | ||
| node.props["aria-label"].startsWith("Copy") | ||
| ); | ||
| }); | ||
|
|
||
| // Should find 2 copy buttons (one for EVM, one for SOL) | ||
| expect(copyButtons.length).toBe(2); |
There was a problem hiding this comment.
Insufficient interaction testing for CopyButton
The test only verifies the presence of copy buttons but does not check whether the copyToClipboard function is called when the button is clicked. This misses an important behavioral aspect and reduces test coverage.
Recommended solution:
Add a test that simulates a click on the copy button and asserts that mockUseApp.copyToClipboard is called:
copyButtons[0].props.onClick();
expect(mockUseApp.copyToClipboard).toHaveBeenCalled();| /> | ||
| </div> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
Security: Potential Exposure of Wallet Addresses
The code displays EVM and Solana wallet addresses in the UI (lines 204-227). If the Header component is rendered for unauthenticated or unauthorized users, this could expose sensitive information. Ensure that only authorized users can access this component or that wallet addresses are not displayed unless the user is authenticated.
Recommended Solution:
- Confirm that the parent application enforces authentication and authorization before rendering this component.
- If not, add explicit checks to prevent wallet address exposure to unauthorized users.
| const handleCopy = async (e: React.MouseEvent) => { | ||
| e.stopPropagation(); | ||
| await copyToClipboard(value); | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| }; |
There was a problem hiding this comment.
The handleCopy function does not handle errors from the asynchronous copyToClipboard operation. If the clipboard operation fails, the UI will still indicate success, which is misleading. Consider wrapping the clipboard call in a try/catch block and providing user feedback on failure:
try {
await copyToClipboard(value);
setCopied(true);
setTimeout(() => setCopied(false), 2000);
} catch (err) {
// Optionally set an error state or notify the user
}| e.stopPropagation(); | ||
| await copyToClipboard(value); | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); |
There was a problem hiding this comment.
The use of setTimeout to reset the copied state does not account for the component unmounting before the timeout fires. This can lead to a memory leak or React warning about setting state on an unmounted component. To address this, clear the timeout in a useEffect cleanup function or use a ref to track mounted state.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a CopyButton component to provide visual feedback when copying wallet addresses, which is a great UX improvement. The implementation is clean and effectively refactors the Header component.
I've left a couple of suggestions:
- In
CopyButton.tsx, I've pointed out a potential memory leak withsetTimeoutand suggested usinguseEffectfor proper cleanup. - In
tsconfig.json, I've noted a small improvement to keep the configuration tidy.
Overall, great work on improving the user experience.
| const handleCopy = async (e: React.MouseEvent) => { | ||
| e.stopPropagation(); | ||
| await copyToClipboard(value); | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| }; |
There was a problem hiding this comment.
This implementation has a potential memory leak. If the CopyButton component unmounts before the 2-second timeout completes, setCopied(false) will be called on an unmounted component. This causes a React warning and is not best practice.
To fix this, you should manage the timer with a useEffect hook. This provides a cleanup function to clear the timeout when the component unmounts.
Here's a suggested refactoring (you'll also need to import useEffect from react):
useEffect(() => {
if (!copied) return;
const timerId = setTimeout(() => {
setCopied(false);
}, 2000);
return () => clearTimeout(timerId);
}, [copied]);
const handleCopy = async (e: React.MouseEvent) => {
e.stopPropagation();
await copyToClipboard(value);
setCopied(true);
};| "types": ["vite/client", "bun-types"] | ||
| }, | ||
| "include": ["src/**/*.ts", "src/**/*.tsx"], | ||
| "include": ["src/**/*.ts", "src/**/*.tsx", "test/**/*.ts", "test/**/*.tsx"], |
There was a problem hiding this comment.
The test file Header.test.tsx is located inside the src/components directory, which is already covered by the "src/**/*.tsx" pattern. The addition of "test/**/*.ts" and "test/**/*.tsx" seems unnecessary if there isn't a separate test directory at the root of the apps/app project.
To keep the configuration clean, you could remove the added test paths.
| "include": ["src/**/*.ts", "src/**/*.tsx", "test/**/*.ts", "test/**/*.tsx"], | |
| "include": ["src/**/*.ts", "src/**/*.tsx"], |
1 participant
This PR improves the UX of copying wallet addresses in the header by providing immediate visual feedback.
Changes:
CopyButtonhandles the copy action and displays a temporary "copied" state with a checkmark.CopyButton.PR created automatically by Jules for task 13139731818391130379 started by @Dexploarer