fix: wallet bugs #650
fix: wallet bugs #650
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces several modifications across multiple components, focusing on UI improvements and utility function enhancements. The changes primarily involve replacing profile navigation buttons with a new Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
src/components/Global/PaymentsFooter/index.tsx (1)
Navigation path inconsistency detected
The change from '/profile' to '/history' in PaymentsFooter's default prop appears to be inconsistent with the rest of the codebase:
- Multiple components still use '/profile' for user-related navigation (Header, Login, LinkAccount)
- No other components are using '/history' as a navigation path
- This inconsistency might lead to unexpected user navigation flows
🔗 Analysis chain
Line range hint
1-24: LGTM! Verify navigation changes across the application.The PaymentsFooter component is well-structured. The change from '/profile' to '/history' aligns with the PR objectives.
Let's verify this navigation change is consistent across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to '/profile' in navigation rg -g '*.{tsx,ts,jsx,js}' "href.*['\"]\/profile['\"]"Length of output: 159
Script:
#!/bin/bash # Search for '/profile' references with proper escaping rg -g '*.{tsx,ts,jsx,js}' 'href=.*"/profile"' rg -g '*.{tsx,ts,jsx,js}' "href=.*'/profile'" # Search for '/history' references rg -g '*.{tsx,ts,jsx,js}' 'href=.*"/history"' rg -g '*.{tsx,ts,jsx,js}' "href=.*'/history'" # Find PaymentsFooter component usage rg -g '*.{tsx,ts,jsx,js}' '<PaymentsFooter' -B 1 -A 3Length of output: 4546
🧹 Nitpick comments (9)
src/components/Claim/Link/Onchain/Confirm.view.tsx (2)
Line range hint
183-217: Consider extracting route information into a separate component.The route information display contains complex nested conditionals and repeated chain lookups. Consider extracting this into a separate component to improve maintainability and reusability.
Example refactor:
type RouteInfoProps = { selectedRoute: typeof selectedRoute; claimLinkData: typeof claimLinkData; supportedSquidChainsAndTokens: typeof supportedSquidChainsAndTokens; }; const RouteInfo: React.FC<RouteInfoProps> = ({ selectedRoute, claimLinkData, supportedSquidChainsAndTokens }) => { const fromChain = consts.supportedPeanutChains.find( (chain) => chain.chainId === selectedRoute.route.params.fromChain )?.name; const toChain = supportedSquidChainsAndTokens[selectedRoute.route.params.toChain]?.axelarChainName; return ( <div className="flex w-full flex-row items-center justify-between text-h8 text-grey-1"> <div className="flex w-max flex-row items-center justify-center gap-1"> <Icon name={'forward'} className="h-4 fill-grey-1" /> <label className="font-bold">Route</label> </div> <span className="flex flex-row items-center justify-center gap-1 text-center text-sm font-normal leading-4"> {fromChain} <Icon name={'arrow-next'} className="h-4 fill-grey-1" /> {toChain} <MoreInfo text={`You are bridging ${claimLinkData.tokenSymbol.toLowerCase()} on ${fromChain} to ${selectedRoute.route.estimate.toToken.symbol.toLowerCase()} on ${toChain}.`} /> </span> </div> ); };
Line range hint
218-226: Consider making fee information configurable.The hardcoded fee value and sponsorship message could be problematic if the sponsorship policy changes. Consider moving these to configuration or constants.
Example refactor:
// In constants file export const TRANSACTION_FEES = { amount: '0.00', currency: 'USD', isSponsored: true, sponsorshipMessage: 'This transaction is sponsored by peanut! Enjoy!' }; // In component <div className="flex w-full flex-row items-center justify-between text-h8 text-grey-1"> <div className="flex w-max flex-row items-center justify-center gap-1"> <Icon name={'gas'} className="h-4 fill-grey-1" /> <label className="font-bold">Fees</label> </div> <span className="flex flex-row items-center justify-center gap-1 text-center text-sm font-normal leading-4"> ${TRANSACTION_FEES.amount} {TRANSACTION_FEES.isSponsored && ( <MoreInfo text={TRANSACTION_FEES.sponsorshipMessage} /> )} </span> </div>src/components/Claim/Link/Onchain/Success.view.tsx (1)
Line range hint
106-116: Consider grouping related navigation elements.The PaymentsFooter and Discord feedback link serve different purposes but are placed next to each other. Consider wrapping them in a semantic container with appropriate spacing.
+ <div className="flex flex-col gap-3"> <PaymentsFooter /> <label className="text-start text-h9 font-normal"> We would like to hear from your experience. Hit us up on{' '} <a className="cursor-pointer text-black underline dark:text-white" target="_blank" href="https://discord.gg/BX9Ak7AW28" > Discord! </a> </label> + </div>src/components/Create/Link/Success.view.tsx (1)
132-132: LGTM! Consider adding visual separation for PaymentsFooter.The PaymentsFooter is well-integrated within the button group. However, since it serves a different purpose than the share and transaction buttons, consider adding visual separation.
<Link className="w-full" target="_blank" href={`${explorerUrlWithTx}`}> <Button variant="dark"> Transaction hash <Icon name="external" className="h-4 fill-grey-1" /> </Button> </Link> + <div className="border-t border-grey-4 pt-2"> <PaymentsFooter /> + </div>src/components/Request/Pay/Views/Success.view.tsx (2)
Line range hint
35-71: Consider extracting duplicate transaction submission logic.There's significant code duplication in the transaction submission logic between the two useEffect hooks. Consider extracting this into a reusable function:
+ const submitAndSaveRequestLinkFulfillment = (hash: string) => { + peanut.submitRequestLinkFulfillment({ + chainId: requestLinkData.chainId, + hash, + payerAddress: address ?? '', + link: requestLinkData.link, + apiUrl: '/api/proxy/patch/', + amountUsd: (Number(requestLinkData.tokenAmount) * (tokenPriceData?.price ?? 0)).toFixed(2), + }) + utils.saveRequestLinkFulfillmentToLocalStorage({ + details: { + ...requestLinkData, + destinationChainFulfillmentHash: hash, + createdAt: new Date().toISOString(), + }, + link: requestLinkData.link, + }) + } useEffect(() => { if (explorerUrlDestChainWithTxHash) { - peanut.submitRequestLinkFulfillment({ - chainId: requestLinkData.chainId, - hash: explorerUrlDestChainWithTxHash.transactionId, - payerAddress: address ?? '', - link: requestLinkData.link, - apiUrl: '/api/proxy/patch/', - amountUsd: (Number(requestLinkData.tokenAmount) * (tokenPriceData?.price ?? 0)).toFixed(2), - }) - utils.saveRequestLinkFulfillmentToLocalStorage({ - details: { - ...requestLinkData, - destinationChainFulfillmentHash: explorerUrlDestChainWithTxHash.transactionId, - createdAt: new Date().toISOString(), - }, - link: requestLinkData.link, - }) + submitAndSaveRequestLinkFulfillment(explorerUrlDestChainWithTxHash.transactionId) setLoadingState('Idle') } }, [explorerUrlDestChainWithTxHash, requestLinkData, address]) useEffect(() => { if (!isXChain && !utils.areEvmAddressesEqual(selectedTokenAddress, requestLinkData.tokenAddress)) { - peanut.submitRequestLinkFulfillment({ - chainId: requestLinkData.chainId, - hash: transactionHash, - payerAddress: address ?? '', - link: requestLinkData.link, - apiUrl: '/api/proxy/patch/', - amountUsd: (Number(requestLinkData.tokenAmount) * (tokenPriceData?.price ?? 0)).toFixed(2), - }) - utils.saveRequestLinkFulfillmentToLocalStorage({ - details: { - ...requestLinkData, - destinationChainFulfillmentHash: transactionHash, - createdAt: new Date().toISOString(), - }, - link: requestLinkData.link, - }) + submitAndSaveRequestLinkFulfillment(transactionHash) } }, [isXChain, requestLinkData, transactionHash, address, selectedTokenAddress])
Line range hint
144-150: Move Discord URL to constants.The Discord URL should be moved to a constants file to maintain consistency and make updates easier.
+ // In src/constants/social.ts + export const DISCORD_INVITE_URL = 'https://discord.gg/BX9Ak7AW28' - href="https://discord.gg/BX9Ak7AW28" + href={DISCORD_INVITE_URL}src/utils/__tests__/general.utils.test.ts (1)
74-175: Consider adding more test cases for comprehensive coverage.The test suite for
formatExtendedNumberis well-structured and covers most scenarios. Consider adding these additional test cases for even better coverage:
- Zero with different formats
- Very large numbers beyond trillion
- Very small decimal numbers
- Numbers at exact power boundaries
describe('formatExtendedNumber', () => { // ... existing test cases ... + describe('additional edge cases', () => { + it('should handle zero with different formats', () => { + expect(formatExtendedNumber('0.00')).toBe('0') + expect(formatExtendedNumber('0.000')).toBe('0') + expect(formatExtendedNumber('-0.00')).toBe('0') + }) + + it('should handle very large numbers beyond trillion', () => { + expect(formatExtendedNumber('1234567890000000')).toBe('1234.57T') + expect(formatExtendedNumber('9999999999999999')).toBe('9999.99T') + }) + + it('should handle very small decimal numbers', () => { + expect(formatExtendedNumber('0.0000001')).toBe('0.0000001') + expect(formatExtendedNumber('0.000000001')).toBe('0') + }) + + it('should handle numbers at exact power boundaries', () => { + expect(formatExtendedNumber('999999999')).toBe('1B') + expect(formatExtendedNumber('999499999')).toBe('999.5M') + }) + }) })src/utils/general.utils.ts (2)
952-965: Remove duplicate JSDoc comments.The JSDoc comments for
formatExtendedNumberare duplicated. Keep only one instance of the documentation.-/** - * Formats a number to use K, M, B, T suffixes if it exceeds 6 characters in length. - * Uses the formatAmount function to format the number before adding a suffix. - * - * @param amount - The number or string to format. - * @returns A formatted string with appropriate suffix. - */
966-1014: Consider memoizing frequently used values.For better performance, consider memoizing the results of
formatExtendedNumberfor frequently accessed values, especially in the context of wallet balances that might not change often.+const memoizedFormatExtendedNumber = (() => { + const cache = new Map<string, string>() + const MAX_CACHE_SIZE = 1000 + + return (amount: string | number): string => { + const key = amount.toString() + if (cache.has(key)) { + return cache.get(key)! + } + + const result = formatExtendedNumber(amount) + + if (cache.size >= MAX_CACHE_SIZE) { + // Clear the cache when it gets too large + cache.clear() + } + + cache.set(key, result) + return result + } +})()Consider exporting both the memoized and non-memoized versions:
export const formatExtendedNumber = (amount: string | number): string => { // ... existing implementation ... } + +export const formatExtendedNumberMemoized = memoizedFormatExtendedNumber
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/components/Claim/Link/Initial.view.tsx(1 hunks)src/components/Claim/Link/Onchain/Confirm.view.tsx(5 hunks)src/components/Claim/Link/Onchain/Success.view.tsx(2 hunks)src/components/Create/Link/Success.view.tsx(2 hunks)src/components/Global/PaymentsFooter/index.tsx(1 hunks)src/components/Home/WalletCard.tsx(2 hunks)src/components/Request/Create/Views/Success.view.tsx(2 hunks)src/components/Request/Pay/Views/Success.view.tsx(2 hunks)src/utils/__tests__/general.utils.test.ts(2 hunks)src/utils/general.utils.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/Claim/Link/Initial.view.tsx
🔇 Additional comments (8)
src/components/Claim/Link/Onchain/Confirm.view.tsx (3)
115-118: LGTM! UI improvements enhance visual consistency.The addition of
disableWalletHeaderand centering classes align with the PR objectives and improve the visual presentation.
Line range hint
151-162: LGTM! Improved text alignment enhances readability.The left alignment of cross-chain claim details provides better visual hierarchy and readability.
163-170: LGTM! Consistent formatting prevents UI breaks.The left alignment maintains visual consistency, and the use of
formatTokenAmounthelps prevent UI breaks from improperly formatted numbers.src/components/Request/Create/Views/Success.view.tsx (1)
Line range hint
1-22: LGTM! Clean integration of PaymentsFooter.The success view maintains a clear and logical flow, with the PaymentsFooter properly positioned at the bottom of the card.
src/components/Request/Pay/Views/Success.view.tsx (3)
2-4: LGTM! Clean import changes.The removal of Button and Icon imports in favor of PaymentsFooter aligns well with the component's updated navigation pattern.
Line range hint
82-95: LGTM! Well-implemented loading state.The loading state provides good user feedback with:
- Animated spinner
- Screen reader support
- Clear status message
152-152: Verify PaymentsFooter default props.The PaymentsFooter is used without any props. Please verify if the default href='/history' is the intended behavior for this success view.
✅ Verification successful
Default PaymentsFooter behavior is appropriate for success view
The PaymentsFooter component's default href='/history' is the intended behavior for success views, consistently used across multiple success-related components to direct users to their payment history after completing an action.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check PaymentsFooter implementation and usage ast-grep --pattern 'export const PaymentsFooter = ({ href = $_' && \ rg -A 5 '<PaymentsFooter'Length of output: 4199
Script:
#!/bin/bash # Find PaymentsFooter component definition fd PaymentsFooter.tsx && \ ast-grep --pattern 'function PaymentsFooter' && \ ast-grep --pattern 'const PaymentsFooter: React.FC'Length of output: 124
Script:
#!/bin/bash # Find PaymentsFooter component definition with broader search rg -l "PaymentsFooter" src/components && \ rg "export (const|function|default|class) .*PaymentsFooter" -A 10Length of output: 1552
src/components/Home/WalletCard.tsx (1)
160-160: Check for duplicate dollar sign in the formatted balance.The template literal adds a
$prefix, butprintableUsdcmight already include the currency symbol. Verify if this leads to duplicate dollar signs in the UI.
|
|
||
| /** | ||
| * Formats a number to use K, M, B, T suffixes if it exceeds 6 characters in length. | ||
| * Uses the formatAmount function to format the number before adding a suffix. | ||
| * | ||
| * @param amount - The number or string to format. | ||
| * @returns A formatted string with appropriate suffix. | ||
| */ | ||
| /** |
There was a problem hiding this comment.
oops, will fix it
| * Formats a number to use K, M, B, T suffixes if it exceeds 6 digits in length. | ||
| * Uses the formatAmount function to format the number before adding a suffix. | ||
| * | ||
| * @param amount - The number or string to format. | ||
| * @returns A formatted string with appropriate suffix. | ||
| */ | ||
| export const formatExtendedNumber = (amount: string | number): string => { |
There was a problem hiding this comment.
Thought: im 99.9999999999999999999% sure a package exists for this. On the other hand its a simple function and we do have tests for it
There was a problem hiding this comment.
nit: code organization wise this should be in the file closer to the other "format...." functions, not at the end of utils.ts on its own
There was a problem hiding this comment.
lmao, i'm gonna download the isEven package for once sure, just waiting for the day we need it
- Noted, will re-arrange this in some other future pr
| export const printableAddress = (address: string): string => { | ||
| if (address.endsWith('.eth')) return address | ||
| return shortenAddressLong(address) | ||
| } |
There was a problem hiding this comment.
Q: why delete these? Are they unused?
There was a problem hiding this comment.
seems like it. nice. What tool do you use for this?
There was a problem hiding this comment.
did it manually 💀
| it('should handle empty string', () => { | ||
| expect(formatExtendedNumber('')).toBe('0') | ||
| }) | ||
|
|
||
| it('should handle invalid string input', () => { | ||
| expect(formatExtendedNumber('invalid')).toBe('0') | ||
| }) | ||
|
|
||
| it('should handle NaN', () => { | ||
| expect(formatExtendedNumber(NaN)).toBe('0') | ||
| }) | ||
|
|
There was a problem hiding this comment.
Q: whats youur reasoning why it should format these and not throw instead?
There was a problem hiding this comment.
umm rendering NaN in ui might not be a better choice imo
| it('should format decimal numbers exceeding 6 total digits', () => { | ||
| expect(formatExtendedNumber(1234.567)).toBe('1.23K') | ||
| expect(formatExtendedNumber(12345.67)).toBe('12.35K') |
There was a problem hiding this comment.
v nice, was worried about these cases, lgtm
| <Card> | ||
| <Card.Header> | ||
| <Card.Title> | ||
| <Card.Title className="mx-auto text-center"> |
There was a problem hiding this comment.
Q: shouldn't the title be left-aligned?
There was a problem hiding this comment.
nah nah, its the token stuff on confirm screen, needs to be centred, and to mention we revamping it to current prod designs so confirm views gonna change
| </span> | ||
| ) : ( | ||
| `$ ${printableUsdc(wallet.balance)}` | ||
| `$ ${formatExtendedNumber(printableUsdc(wallet.balance))}` |
There was a problem hiding this comment.
Thought: honestly im not sure printableUsdc is a good function. Feels like we should merge it with our formatting functions. on top off that it doesnt work on bnb where usdc has 18 decimals
There was a problem hiding this comment.
umm, alright that's a valid point, ideally my intention is to use only 1 or 2 same formatting fns moving forward and not use any other, checked the printableUsdc code, it's using viem's formatUnits method only for 6 decimals, I'll check this on my end and update
There was a problem hiding this comment.
gonna look into this one, and if there's any solid action item, I'll take care of it in another pr
| return ( | ||
| <div> | ||
| <FlowHeader onPrev={onPrev} disableBackBtn={isLoading} /> | ||
| <FlowHeader onPrev={onPrev} disableBackBtn={isLoading} disableWalletHeader /> |
There was a problem hiding this comment.
Q: Shouldn't FlowHeader and WalletHeader be separate? And both be added in the confirm view
There was a problem hiding this comment.
very nitpicky, but id suggest:
WalletHeader
NavHeader
And then we can combine them into one FlowHeader or smth. That's how i'd think about these semantically
There was a problem hiding this comment.
@Hugo0 FlowHeader has wallet header called inside it, along with the cta, i believe in future this will need update based on the new designs that will be made, so its fine for now
formatExtendedNumberfn to format wallet balances on wallet card to prevent ui from breakingformatExtendedNumberfnfixed below issues
Summary by CodeRabbit
Release Notes
New Features
PaymentsFootercomponent to multiple success viewsformatExtendedNumberutility for enhanced number formattingImprovements
/profileto/historyVisual Changes
Code Cleanup