Repository navigation
fix(tokenlogo): flag clip-path ids collide when a flag renders more than once - #1037
Conversation
Hardcoded clipPath ids (us/br/cu/gb-flag-circle) collide when the same flag renders more than once on a page (e.g. transaction lists or swap routes), so the second instance would reference the wrong clip path. Each flag now generates a unique id per component instance via useId and points its <g clipPath> at that scoped id. (cherry picked from commit d0bdd3c)
|
Warning Review limit reachedNext included review available in 53 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
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 |
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review
Correct diagnosis and clean implementation. No protocol-critical paths touched.
Fix (src/components/TokenLogo.tsx)
useClipId hook — correct (TokenLogo.tsx:7-9).
useId() (React 18, confirmed in package.json) generates a per-render-tree-stable, document-unique id. The global colon-strip (.replace(/:/g, '')) is the right approach: React's :r0: format produces readable ids like us-flag-circle-r0, and a global replace stays robust if the internal format ever gains more colons. The hook is called before return in each component function, satisfying Rules of Hooks.
Three flags updated (TokenLogo.tsx:116,190,239) — US, Brazil, UK. Switzerland, China, EU, Japan, and Bitcoin/Tether/USDC logos either use a bounding <circle> or <g> without a clipPath reference, so they are correctly left untouched. The comment about the Cuban flag / #991 branch is a useful heads-up for reviewers of that PR.
Tests (src/test/components/token-logo.test.tsx)
Well-structured. Three positive aspects:
-
Uniqueness assertion (line ~43-52): renders two of each flag, collects
clipPath[id]values per prefix, and asserts both count = 2 andSetsize = 2. Catches the regression directly. -
Scoping assertion (line ~54-68): for every
<svg>in the rendered output, resolves theclip-pathid and asserts the matchingclipPathlives inside that svg. This is the test that would have caught the original bug — good that it is the explicit regression case, not just a snapshot. -
tokenLogoTickerForTickercoverage (lines ~5-19): tests normalisation (trim + uppercase) and the unknown-ticker undefined path. The.toBe()comparison is correct for string value equality.
Swap test update (src/test/screens/wallet/swap.test.tsx:178)
The old document.querySelector('#br-flag-circle') assertion was testing an implementation detail that this PR deliberately invalidates. Switching to clipPath[id^="br-flag-circle"] preserves the intent (the Brazil flag rendered) while being compatible with dynamic ids.
Cross-repo impact
No consumers of TokenLogo, us-flag-circle, br-flag-circle, or gb-flag-circle exist outside the wallet repo — confirmed by grep across all SDK clones.
Minor observations (non-blocking)
UnitedStatesFlagLogois exported fromTokenLogo.tsxbut the new test file does not import it directly — it reaches it through<TokenLogo ticker='USD' />. Coverage is equivalent; just noting it in case someone later wants a directUnitedStatesFlagLogotest.- The
useClipIdhelper is file-private (noexport), which is the right default. If a future component in a different file needs the same treatment, it can be promoted to a shared util then.
LGTM.
Closes #1035
Problem
Three flag logos in
src/components/TokenLogo.tsx—UnitedStatesFlagLogo,BrazilFlagLogoandUnitedKingdomFlagLogo— each define aclipPathwith a hardcoded id and reference it throughurl(#id).SVG fragment references resolve against the whole document, not the local
<svg>. So when the same flag renders more than once on a page, the second instance'surl(#us-flag-circle)resolves to the first element with that id in document order.This is reachable in normal use:
TokenLogois rendered byTransactionsList(once per transaction),SwapRouteIcon(once per route hop),AssetCard,Swap/Index,Send/Form,BitcoinDetailandSettings/Currency. A transaction list with two USD entries, or a multi-hop swap, produces duplicate ids.The visual result happens to be correct today, because every instance of a given flag defines an identical circle. That is what makes this worth fixing now rather than later: it is invalid HTML, and the next edit that gives one flag a different clip shape will silently start clipping against the wrong element. Duplicate ids also break
document.getElementByIdfor anything that reaches for them.Fix
Scope each id to its component instance with React's
useId(), keeping a readable prefix so the ids stay greppable:The colons are stripped because
useId()returns values like:r0:, which make theurl(#…)reference awkward to read and to search for.Tests
New
src/test/components/token-logo.test.tsx:clipPathids (checked for all three flags)g[clip-path]must resolve to aclipPathdefined inside its own<svg>The second test is the one that catches the actual bug: it fails when
useClipIdis replaced by a constant, which is how the fix was verified.src/test/screens/wallet/swap.test.tsxasserted the literal id#br-flag-circle; it now matches by prefix instead.Note on scope
This is deliberately limited to the three flags that exist on
master. The Cuban flag and theCUPticker are part of the fiat work in #991, so they are not touched here — that branch carries the sameuseIdtreatment for the flags it adds.Validation
pnpm test— 84/84 files, 748/748 testspnpm run format:check— cleanpnpm run lint— cleanpnpm exec tsc --noEmit— clean apart from one pre-existing error insrc/providers/lnSwaps.tsx:99, present onmasterpnpm build— succeeds