Skip to content

🎨 Palette: Add copy feedback to wallet addresses #152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Dexploarer wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

🎨 Palette: Add copy feedback to wallet addresses #152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions apps/app/src/components/Header.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,25 +36,16 @@ describe("Header", () => {
lifecycleAction: null,
handlePauseResume: vi.fn(),
handleRestart: vi.fn(),
copyToClipboard: vi.fn(),
setTab: vi.fn(),
dropStatus: null,
loadDropStatus: vi.fn(),
registryStatus: null,
copyToClipboard: vi.fn(), // Needed for CopyButton
};

// @ts-expect-error - test uses a narrowed subset of the full app context type.
vi.spyOn(AppContext, "useApp").mockReturnValue(mockUseApp);
Comment on lines 46 to 47
Copy link

@codereviewbot-ai codereviewbot-ai bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> = { ... };


// We need to render the component.
// Note: Since we are in a non-browser environment (happy-dom/jsdom might not be set up fully for standard React testing library in this repo's specific config),
// we will check if we can use react-test-renderer or if we should rely on a basic snapshot/class check.
// However, the user's package.json includes "react-test-renderer".
// Let's try react-test-renderer first as it avoids DOM emulation issues if not configured.

// Actually, let's stick to the plan of using what's available.
// The previous check showed "react-test-renderer": "^19.0.0".

let testRenderer: ReactTestRenderer | null = null;
await act(async () => {
testRenderer = create(<Header />);
Expand All @@ -68,7 +59,6 @@ describe("Header", () => {
node.props.className.includes(className);

// Find the wallet wrapper
// It has className "wallet-wrapper relative inline-flex shrink-0 group"
const walletWrapper = root.findAll((node: ReactTestInstance) =>
hasClass(node, "wallet-wrapper"),
);
Expand All @@ -77,12 +67,23 @@ describe("Header", () => {
expect(walletWrapper[0].props.className).toContain("group");

// Find the wallet tooltip
// It should have className containing "group-hover:block"
const walletTooltip = root.findAll((node: ReactTestInstance) =>
hasClass(node, "wallet-tooltip"),
);

expect(walletTooltip.length).toBe(1);
expect(walletTooltip[0].props.className).toContain("group-hover:block");

// Verify CopyButtons are rendered
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);
Comment on lines +78 to +87
Copy link

@codereviewbot-ai codereviewbot-ai bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

});
});
36 changes: 9 additions & 27 deletions apps/app/src/components/Header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
import { useEffect } from "react";
import { useApp } from "../AppContext";
import { useBugReport } from "../hooks/useBugReport";
import { CopyButton } from "./shared/CopyButton";

export function Header() {
const {
Expand All @@ -25,7 +26,6 @@ export function Header() {
lifecycleAction,
handlePauseResume,
handleRestart,
copyToClipboard,
setTab,
dropStatus,
loadDropStatus,
Expand Down Expand Up @@ -204,19 +204,10 @@ export function Header() {
<code className="font-mono flex-1 truncate">
{evmShort}
</code>
<button
type="button"
onClick={(e) => {
e.stopPropagation();
const evmAddress = walletAddresses?.evmAddress;
if (evmAddress) {
copyToClipboard(evmAddress);
}
}}
className="px-1.5 py-1 border border-border bg-bg text-[10px] font-mono cursor-pointer hover:border-accent hover:text-accent"
>
copy
</button>
<CopyButton
value={walletAddresses?.evmAddress ?? ""}
label="copy"
/>
</div>
)}
{solShort && (
Expand All @@ -227,19 +218,10 @@ export function Header() {
<code className="font-mono flex-1 truncate">
{solShort}
</code>
<button
type="button"
onClick={(e) => {
e.stopPropagation();
const solanaAddress = walletAddresses?.solanaAddress;
if (solanaAddress) {
copyToClipboard(solanaAddress);
}
}}
className="px-1.5 py-1 border border-border bg-bg text-[10px] font-mono cursor-pointer hover:border-accent hover:text-accent"
>
copy
</button>
<CopyButton
value={walletAddresses?.solanaAddress ?? ""}
label="copy"
/>
</div>
)}
</div>
Copy link

@codereviewbot-ai codereviewbot-ai bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
50 changes: 50 additions & 0 deletions apps/app/src/components/shared/CopyButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { Check, Copy } from "lucide-react";
import { useState } from "react";
import { useApp } from "../../AppContext";

interface CopyButtonProps {
value: string;
label?: string;
className?: string;
}

export function CopyButton({
value,
label = "copy",
className = "",
}: CopyButtonProps) {
const { copyToClipboard } = useApp();
const [copied, setCopied] = useState(false);

const handleCopy = async (e: React.MouseEvent) => {
e.stopPropagation();
await copyToClipboard(value);
setCopied(true);
setTimeout(() => setCopied(false), 2000);
Copy link

@codereviewbot-ai codereviewbot-ai bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
Comment on lines +19 to +24
Copy link

@codereviewbot-ai codereviewbot-ai bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Comment on lines +19 to +24
Copy link

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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);
  };


return (
<button
type="button"
onClick={handleCopy}
className={`px-1.5 py-1 border border-border bg-bg text-[10px] font-mono cursor-pointer transition-all duration-200 inline-flex items-center gap-1 ${
copied
? "border-ok text-ok bg-ok-subtle"
: "hover:border-accent hover:text-accent"
} ${className}`}
aria-label={copied ? "Copied" : `Copy ${label}`}
>
{copied ? (
<>
<Check className="w-3 h-3" />
<span>copied</span>
</>
) : (
<>
<Copy className="w-3 h-3" />
<span>{label}</span>
</>
)}
</button>
);
}
4 changes: 2 additions & 2 deletions apps/app/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
"forceConsistentCasingInFileNames": true,
"experimentalDecorators": true,
"useUnknownInCatchVariables": true,
"types": ["vite/client"]
"types": ["vite/client", "bun-types"]
},
"include": ["src/**/*.ts", "src/**/*.tsx"],
"include": ["src/**/*.ts", "src/**/*.tsx", "test/**/*.ts", "test/**/*.tsx"],
Copy link

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
"include": ["src/**/*.ts", "src/**/*.tsx", "test/**/*.ts", "test/**/*.tsx"],
"include": ["src/**/*.ts", "src/**/*.tsx"],

"exclude": ["node_modules", "dist", "ios", "android", "electron"]
}
Loading