fix: Symbols not showing in routing breakdown #4221
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
4 Skipped Deployments
|
WalkthroughImage rendering was changed to prefer SVG assets: components now derive an SVG URL from existing PNG URLs and pass both Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/web/components/swap-tool/split-route.tsx (1)
237-254: The string replacement approach assumes consistent chain registry URL structure.The implementation derives SVG URLs by replacing
.pngwith.svgincurrency.coinImageUrl. This approach works if the chain registry consistently places both formats at the same path with different extensions.Potential considerations:
- Case sensitivity: The regex
/\.png$/won't match.PNG(uppercase)- URL structure assumption: If some assets have PNGs but not SVGs at the derived path, this will generate 404 requests for non-existent SVGs
- Non-.png URLs: If
coinImageUrldoesn't end with.png(e.g., already an SVG or different format), bothsvgandpngproperties will receive the same URLThe approach is safe due to optional chaining and EntityImage's fallback logic, but it relies on the chain registry's URL conventions being consistent.
If chain registry URL structure varies, consider checking response headers or using a more robust URL derivation method. Otherwise, the current approach is acceptable given the PR's assertion that "SVGs are more widely available in the chain registry."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/web/components/swap-tool/split-route.tsx(3 hunks)packages/web/components/ui/entity-image.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-23T16:36:41.685Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/alert/prettify.ts:43-44
Timestamp: 2024-11-23T16:36:41.685Z
Learning: In `packages/web/components/alert/prettify.ts`, when handling overspend error messages related to `uusdc`, assuming 6 decimal places is acceptable because `uusdc` uses 6 decimals.
Applied to files:
packages/web/components/swap-tool/split-route.tsx
🧬 Code graph analysis (1)
packages/web/components/swap-tool/split-route.tsx (1)
packages/web/components/ui/entity-image.tsx (1)
EntityImage(9-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Summary
🔇 Additional comments (2)
packages/web/components/swap-tool/split-route.tsx (1)
56-56: LGTM! Width fix addresses the display issue.Adding explicit width (
w-7) to match the height ensures the logo containers maintain proper dimensions and aspect ratio, which directly addresses the PR objective of fixing logos not showing in the routing breakdown.Also applies to: 72-72
packages/web/components/ui/entity-image.tsx (1)
23-23: Ensure EntityImage retries PNG when an SVG load fails.split-route.tsx creates an SVG path by replacing
.png→.svgand passes both svg & png to EntityImage, while the component currently selects the URL withlogoURIs.svg || logoURIs.png(packages/web/components/ui/entity-image.tsx ~line 23). If the chosen SVG 404s, verify the component’s onError logic — it must try the PNG before falling back to the letter placeholder. Either implement a progressive fallback (SVG → PNG → letter) by tracking attempted sources in state and switching src onError, or confirm the chain-registry guarantees corresponding SVGs for every PNG at the expected path.Files to check: packages/web/components/ui/entity-image.tsx (image selection and onError) and packages/web/components/swap-tool/split-route.tsx (svg construction).
Adds SVG support to asset images in Total Balance and position card components to match the fix applied to swap router logos, ensuring higher quality logo rendering. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/web/components/pool-detail/concentrated.tsx (1)
553-560: Consider applying SVG preference consistently.This
EntityImageusage (lines 553-560) only provides apngURL without attempting SVG derivation, while other usages in this file (lines 503-506) and inexpanded.tsxdo derive SVG URLs.Is there a reason this incentive image shouldn't also prefer SVG? If SVG assets are more widely available in the chain registry (per the PR description), it would be more consistent to apply the same SVG-first approach here.
If SVG preference should be applied here as well, consider:
<EntityImage logoURIs={{ + ...(incentive.coinPerDay.currency.coinImageUrl?.toLowerCase().endsWith('.png') && { + svg: incentive.coinPerDay.currency.coinImageUrl.replace(/\.png$/i, '.svg') + }), png: incentive.coinPerDay.currency.coinImageUrl, }} name={incentive.coinPerDay.currency.coinDenom} symbol={incentive.coinPerDay.currency.coinDenom} width={20} height={20} />Or better yet, use the helper function suggested earlier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/web/components/cards/my-position/expanded.tsx(1 hunks)packages/web/components/pool-detail/concentrated.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Analyze (javascript)
- GitHub Check: test (22.x, web)
- GitHub Check: test (22.x, bridge)
- GitHub Check: test (22.x, stores)
- GitHub Check: test (22.x, tx)
- GitHub Check: test (22.x, bridge)
- GitHub Check: test (22.x, web)
- GitHub Check: wait-for-deployment
- GitHub Check: Summary
What is the purpose of the change:
Asset logos not populating in the swap details
Also noticed this missing under the pools page "Total Balance" display when there are position owned by the connected wallet:
Linear Task
https://linear.app/osmosis/issue/FE-1542/in-and-out-token-logos-show-as-fallback-image-in-swap-widget
Brief Changelog
Adds a width to these logos
Prefers SVG which are more widely available in the chain registry.
Testing and Verifying
This change has been tested locally by rebuilding the website and verified content and links are expected