feat(CopyButton): add HoverTooltip for copy feedback#7458
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (22)
WalkthroughThis PR introduces a new ChangesUI Component Refactoring and Storybook Setup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Deploying swap-dev with
|
| Latest commit: |
7a65902
|
| Status: | 🚫 Build failed. |
|
@kernelwhisperer If the only reason to add BaseUI is to get the better tooltip, I suggest we don't. While I haven't been prioritizing it due to other more important tasks, the Dropdown refactor PR adds |
kernelwhisperer
force-pushed
the
feat/storybook-2
branch
from
4b182c6 to
117ba40
Compare
kernelwhisperer
force-pushed
the
feat/storybook-3
branch
from
75f92f6 to
57bb786
Compare
kernelwhisperer
force-pushed
the
feat/storybook-2
branch
from
e520077 to
6037241
Compare
kernelwhisperer
force-pushed
the
feat/storybook-3
branch
from
d21e0cc to
2ff09bc
Compare
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Hey @kernelwhisperer , thank you!
However, in my PC (Win11+Chrome browser ) the icon does not show a tooltip/copied message
Also, as for the linked task, we have other places where we use 'copy' button
The UI is still different for these buttons...
kernelwhisperer
force-pushed
the
feat/storybook-2
branch
from
dd3b733 to
2cb84b1
Compare
Tree shaking works really great with this library, if you're reaching for a banana you're not getting the whole forest. It might make sense to leverage https://base-ui.com/react/components/menu for your PR, instead of creating a primitive yourself. I think this Tooltip problem is a great example on why we should avoid that. |
|
@elena-zh my bad, it should work now, but only for Affiliate. I will follow up with another PR (very shortly) to replace it everywhere in the app (I will try cow-fi & explorer too). Let's keep this one as is, to make it easier to review. 🙏 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui/src/pure/CopyButton/index.tsx (1)
49-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIcon-only mode can render unnamed buttons and bypass i18n fallback.
When
iconOnlyis used with noidleContent, the control becomes icon-only and relies on callers to passaria-label. That creates an accessibility gap (already visible in current affiliate call sites). Also,"Copy to clipboard"is hardcoded instead of localized.💡 Proposed fix
+import { t } from '@lingui/core/macro' import { Trans } from '@lingui/react/macro' export function CopyButton(props: CopyButtonProps): ReactNode { const { @@ onCopy, onClick, disabled, + 'aria-label': ariaLabel, type, ...rest } = props @@ - const content = isCopied ? iconOnly ? null : <span>{copiedLabel}</span> : idleContent + const content = isCopied ? (iconOnly ? null : <span>{copiedLabel}</span>) : idleContent + const tooltipContent = isCopied ? copiedLabel : <Trans>Copy to clipboard</Trans> + const fallbackAriaLabel = isCopied ? t`Copied` : t`Copy to clipboard` const button = ( <StyledCopyButton type={type ?? 'button'} onClick={handleClick} disabled={disabled} + aria-label={ariaLabel ?? fallbackAriaLabel} $isCopied={isCopied} $color={color} {...rest} @@ return !idleContent && iconOnly ? ( - <NewTooltip content={isCopied ? copiedLabel : 'Copy to clipboard'} placement="top"> + <NewTooltip content={tooltipContent} placement="top"> {button} </NewTooltip> ) : (Also applies to: 82-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/ui/src/pure/CopyButton/index.tsx` around lines 49 - 64, The component currently allows iconOnly buttons with no idleLabel/aria-label and uses a hardcoded English string; update the CopyButton logic so that when iconOnly is true it derives an accessible label from idleLabel or, if missing, a localized fallback (use the existing i18n mechanism that backs copiedLabel, e.g., <Trans> or t) and ensure that the rendered button receives aria-label set to that fallback; adjust places that compute the rendered children/idle content (references: props.iconOnly, props.idleLabel, props.copiedLabel, children) so the fallback label is used for both visible idleLabel and aria-label when necessary, and remove any hardcoded "Copy to clipboard" literal.
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.ts (1)
88-88: ⚡ Quick winType assertion bypasses receiver address validation.
The cast
as \0x${string}` | undefinedforcesorder?.receiver(which can bestring | null | undefined` per the TypeScript errors shown in PR screenshots) to be treated as a hex address. This assertion:
- Assumes
receiveris always0x-prefixed when present (might not be validated upstream)- Converts
nulltoundefinedimplicitlyThe optional chaining
?.namecorrectly guards againstuseENSreturning undefined/null. Consider whether upstream types should be narrowed (e.g., makingreceivera0x${string}type at the source) or adding a runtime check before callinguseENS.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.ts` at line 88, The line setting receiverEnsName bypasses validation by casting order?.receiver to `0x${string}`; instead, validate the receiver at runtime before calling useENS (or tighten upstream types). Change the logic around receiverEnsName/useENS to first check order?.receiver is a non-null string and a valid hex address (e.g., using an address validator like ethers.utils.isAddress or a simple /^0x[0-9a-fA-F]{40}$/.test) and only pass the validated address into useENS; if invalid or null, pass undefined. Ensure you update references to receiverEnsName and keep the optional chaining on the useENS result as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cowswap-frontend/src/legacy/components/Copy/CopyHelper.stories.tsx`:
- Around line 20-24: The IconOnly story removes visible text so it needs an
accessible name; update the IconOnly Story args to include an explicit
accessible label (e.g., add a prop such as ariaLabel or label to the args
alongside children: undefined and hideCopiedLabel: true) so assistive
technologies have a name for the icon; ensure the prop name matches the
CopyHelper/Copy component API (use ariaLabel if the component accepts aria
props, otherwise use the component's label prop).
In `@libs/ui/src/pure/Tooltip/Tooltip.tsx`:
- Line 101: The destructuring of placement is unsafe: replace the single-line
destructure using placement?.split('-') with a guarded approach that defaults
when placement is undefined—e.g., compute parts = placement?.split('-') ?? []
(or provide a default placement like "top-center"), then destructure into side
and align with safe defaults (ensure side and align variables get fallback
values such as side = 'top' and align = 'center'); update the line referencing
placement (the const [side, align = 'center'] = placement?.split('-') as [Side,
Align?]) to use this guarded/defaulted logic so runtime errors are avoided.
---
Outside diff comments:
In `@libs/ui/src/pure/CopyButton/index.tsx`:
- Around line 49-64: The component currently allows iconOnly buttons with no
idleLabel/aria-label and uses a hardcoded English string; update the CopyButton
logic so that when iconOnly is true it derives an accessible label from
idleLabel or, if missing, a localized fallback (use the existing i18n mechanism
that backs copiedLabel, e.g., <Trans> or t) and ensure that the rendered button
receives aria-label set to that fallback; adjust places that compute the
rendered children/idle content (references: props.iconOnly, props.idleLabel,
props.copiedLabel, children) so the fallback label is used for both visible
idleLabel and aria-label when necessary, and remove any hardcoded "Copy to
clipboard" literal.
---
Nitpick comments:
In
`@apps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.ts`:
- Line 88: The line setting receiverEnsName bypasses validation by casting
order?.receiver to `0x${string}`; instead, validate the receiver at runtime
before calling useENS (or tighten upstream types). Change the logic around
receiverEnsName/useENS to first check order?.receiver is a non-null string and a
valid hex address (e.g., using an address validator like ethers.utils.isAddress
or a simple /^0x[0-9a-fA-F]{40}$/.test) and only pass the validated address into
useENS; if invalid or null, pass undefined. Ensure you update references to
receiverEnsName and keep the optional chaining on the useENS result as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 24bd5122-a68b-4685-ae05-ca2a4b47c834
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
.gitignoreapps/cowswap-frontend/package.jsonapps/cowswap-frontend/src/common/pure/AddressInputPanel/hooks/useAddressResolution.tsapps/cowswap-frontend/src/common/pure/ReceiveAmount/index.tsxapps/cowswap-frontend/src/legacy/components/Copy/CopyHelper.stories.tsxapps/cowswap-frontend/src/legacy/components/Copy/CopyHelper.tsxapps/cowswap-frontend/src/legacy/components/Copy/index.tsxapps/cowswap-frontend/src/locales/en-US.poapps/cowswap-frontend/src/modules/affiliate/containers/AffiliatePartnerCodeInfo.tsxapps/cowswap-frontend/src/modules/affiliate/containers/AffiliateTraderCodeInfo.tsxapps/cowswap-frontend/src/modules/affiliate/pure/AffiliatePartner/AffiliatePartnerCodeForm.tsxapps/cowswap-frontend/src/modules/orderProgressBar/hooks/useOrderProgressBarProps.tsapps/explorer/package.jsonapps/storybook/src/main.tsapps/storybook/src/preview.tsxlibs/ui/package.jsonlibs/ui/src/index.tslibs/ui/src/pure/CopyButton/CopyButton.stories.tsxlibs/ui/src/pure/CopyButton/index.tsxlibs/ui/src/pure/Tooltip/Tooltip.stories.tsxlibs/ui/src/pure/Tooltip/Tooltip.tsxlibs/ui/src/pure/Tooltip/index.tsx
| export const IconOnly: Story = { | ||
| args: { | ||
| children: undefined, | ||
| hideCopiedLabel: true, | ||
| }, |
There was a problem hiding this comment.
Add an accessible label for the icon-only story.
IconOnly removes visible text, so the showcased usage should provide an accessible name for assistive tech.
Suggested patch
export const IconOnly: Story = {
args: {
children: undefined,
hideCopiedLabel: true,
+ 'aria-label': 'Copy address',
},
}📝 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 IconOnly: Story = { | |
| args: { | |
| children: undefined, | |
| hideCopiedLabel: true, | |
| }, | |
| export const IconOnly: Story = { | |
| args: { | |
| children: undefined, | |
| hideCopiedLabel: true, | |
| 'aria-label': 'Copy address', | |
| }, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/cowswap-frontend/src/legacy/components/Copy/CopyHelper.stories.tsx`
around lines 20 - 24, The IconOnly story removes visible text so it needs an
accessible name; update the IconOnly Story args to include an explicit
accessible label (e.g., add a prop such as ariaLabel or label to the args
alongside children: undefined and hideCopiedLabel: true) so assistive
technologies have a name for the icon; ensure the prop name matches the
CopyHelper/Copy component API (use ariaLabel if the component accepts aria
props, otherwise use the component's label prop).
|
|
||
| export function NewTooltip(props: NewTooltipProps): ReactNode { | ||
| const { content, children, placement } = props | ||
| const [side, align = 'center'] = placement?.split('-') as [Side, Align?] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the optional prop + unsafe destructuring pattern and find call sites that omit placement.
rg -n "placement\\?: TooltipPlacement|placement\\?\\.split\\('-'\\)" libs/ui/src/pure/Tooltip/Tooltip.tsx
rg -nP --type=tsx "<NewTooltip(?![^>]*\\bplacement=)"Repository: cowprotocol/cowswap
Length of output: 222
🏁 Script executed:
# Find all usages of NewTooltip to see if placement is ever omitted
rg -n "NewTooltip\b" --type ts --type js -A 3 -B 1 | head -100Repository: cowprotocol/cowswap
Length of output: 3473
🏁 Script executed:
# Also check the actual return type and see how it's actually destructured
cat -n libs/ui/src/pure/Tooltip/Tooltip.tsx | sed -n '90,110p'Repository: cowprotocol/cowswap
Length of output: 907
Fix unsafe destructuring of optional placement prop.
Line 101 will throw at runtime when placement is undefined, since placement?.split('-') returns undefined and is destructured immediately. Although the interface declares placement? as optional, the destructuring assumes it's always defined.
Proposed fix
export function NewTooltip(props: NewTooltipProps): ReactNode {
const { content, children, placement } = props
- const [side, align = 'center'] = placement?.split('-') as [Side, Align?]
+ const [side = 'top', align = 'center'] = (placement?.split('-') ?? []) as [Side?, Align?]📝 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 [side, align = 'center'] = placement?.split('-') as [Side, Align?] | |
| const [side = 'top', align = 'center'] = (placement?.split('-') ?? []) as [Side?, Align?] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/ui/src/pure/Tooltip/Tooltip.tsx` at line 101, The destructuring of
placement is unsafe: replace the single-line destructure using
placement?.split('-') with a guarded approach that defaults when placement is
undefined—e.g., compute parts = placement?.split('-') ?? [] (or provide a
default placement like "top-center"), then destructure into side and align with
safe defaults (ensure side and align variables get fallback values such as side
= 'top' and align = 'center'); update the line referencing placement (the const
[side, align = 'center'] = placement?.split('-') as [Side, Align?]) to use this
guarded/defaulted logic so runtime errors are avoided.
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Hey @kernelwhisperer , thank you!
Approving with an agreement to replace the component in other places in a separate PR :)
4 participants
Summary
Fixes (partially) https://www.notion.so/cownation/copy-and-copied-UI-is-a-bit-different-in-comparison-with-other-places-3028da5f04ca80398470f5892dac18a4
Replace the HoverTooltipcomponent with the one from BaseUI 75f92f6NewTooltipand deprecate the old one, this allows us to incrementally adopt and simplify the props interface of the Tooltip component.Before: Try it out live
Screencast.From.2026-05-07.12-45-50.mp4
After: Try it out live
Screencast.From.2026-05-07.14-00-02.mp4
Bundle size
As questioned by Leandro, what about bundle size cost?
Before & After
Rendered: 3.43KB -> 4.00KB - 16% increase
Gzip: 1.06KB -> 1.35KB - 27% increase
But given our app size is 5940KB (or 5.9MB) Gzipped, that's only a 0.0001% increase in total bundle size.
Looks like treeshaking works beautifully, just beware to keep imports like this:
To Test
Note: The arrow is quite broken for small tooltips but that's an existing problem and I will not spend more time on it:
Background
Summary by CodeRabbit
New Features
NewTooltipcomponent as a modern replacement for existing tooltip implementationChores
@storybook/react-viteand@base-ui/reactRefactor
CopyButtoncomponent across affiliate modules