fix: Wallet Loading and Payment Processing Issues in Invoicing Template #251
Conversation
WalkthroughThe pull request introduces several modifications across multiple files in the invoice dashboard and create invoice form packages. The primary focus is on enhancing the payment process by adding balance checking functionality, improving date handling in invoice forms, and making minor styling adjustments. The changes aim to provide better user feedback, particularly around wallet balance verification before invoice payments. Changes
Assessment against linked issues
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 and nitpick comments (2)
shared/utils/formatAddress.ts (2)
Line range hint
1-1: Remove unused import.The
checkAddressimport is no longer used after the validation was removed.-import { checkAddress } from "./checkEthAddress";
8-8: Consider enhancing the address validation.While the falsy check handles undefined/empty cases, it might be worth considering:
- Adding basic format validation for non-empty addresses
- Using a more specific check like
address === undefined || address === ''to avoid returning empty string for "0" address- if (!address) return ""; + if (address === undefined || address === '') return ""; + if (!/^0x[0-9a-fA-F]{40}$/.test(address)) { + console.warn('Invalid Ethereum address format'); + return ""; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shared/utils/formatAddress.ts(1 hunks)
🔇 Additional comments (2)
shared/utils/formatAddress.ts (2)
Line range hint 3-11: LGTM for the core changes.
The simplified error handling aligns with the PR objectives to fix wallet loading issues when the address is empty/undefined. The formatting logic remains correct for valid addresses.
8-10: Verify the impact of relaxed validation.
The function now returns empty string for falsy addresses instead of throwing errors. Let's verify all usage points to ensure this change aligns with the expected behavior in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
679-696: Consider enabling the 'Pay Now' button even when balance is insufficientDisabling the "Pay Now" button when the user has insufficient funds prevents any action. Users might want to proceed to see detailed error messages or take remedial actions like funding their wallet.
Consider allowing the button to remain enabled and handle the insufficient balance case within the
handlePayment()function:-<Button - type="button" - text={...} - padding="px-[12px] py-[6px]" - onClick={handlePayment} - disabled={!hasEnoughBalance} /> +<Button + type="button" + text={...} + padding="px-[12px] py-[6px]" + onClick={handlePayment} />In
handlePayment(), add a check forhasEnoughBalanceand display an appropriate message if the user lacks sufficient funds.
1003-1007: Avoid using!importantin CSS to maintain theming flexibilityUsing
!importantin CSS can make overriding styles difficult and may conflict with theme configurations.Consider removing
!importantand increasing selector specificity if necessary:-:global(.invoice-view-actions button[disabled]) { - opacity: 0.5; - cursor: not-allowed; - background-color: #71717a !important; -} +:global(.invoice-view-actions button[disabled]) { + opacity: 0.5; + cursor: not-allowed; + background-color: #71717a; +}Also, consider using theme variables for
background-colorto maintain consistent styling.packages/invoice-dashboard/src/lib/view-requests.svelte (2)
396-403: Consider adding error handling for decryption operations.While the reactive statements are correctly implemented, the decryption operations could fail. Consider adding try-catch blocks to handle potential errors gracefully.
$: { if (sliderValueForDecryption === "on") { - cipherProvider?.enableDecryption(true); + try { + cipherProvider?.enableDecryption(true); + } catch (error) { + console.error('Failed to enable decryption:', error); + toast.error('Failed to enable decryption'); + } } else { - cipherProvider?.enableDecryption(false); + try { + cipherProvider?.enableDecryption(false); + } catch (error) { + console.error('Failed to disable decryption:', error); + toast.error('Failed to disable decryption'); + } } }
720-721: Consider separating loading and empty state conditions.The current implementation might not handle edge cases well. Consider separating the conditions to provide better user feedback.
-{:else if loading} - <DashboardSkeleton /> +{:else if loading} + <DashboardSkeleton /> +{:else if processedRequests.length === 0} + <tr> + <td colspan="11" class="text-center py-4"> + No requests found + </td> + </tr>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(7 hunks)packages/invoice-dashboard/src/lib/view-requests.svelte(4 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
Learnt from: MantisClone
PR: RequestNetwork/web-components#141
File: packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte:0-0
Timestamp: 2024-11-12T14:52:33.204Z
Learning: In `packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte`, mapping payment network IDs to their respective approval functions in the `checkApproval` function is acceptable and can improve readability and maintainability.
Learnt from: MantisClone
PR: RequestNetwork/web-components#141
File: packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte:236-247
Timestamp: 2024-11-18T04:03:25.560Z
Learning: In the `checkApproval` function of `packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte` (TypeScript/Svelte code), remember that the `ETH_FEE_PROXY_CONTRACT` payment network ID does not require approval because Ether payments do not need ERC20 approvals.
🔇 Additional comments (5)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
374-414: Validate numerical conversions and error handling in checkBalance()
The checkBalance() function accurately checks the user's balance and updates hasEnoughBalance and userBalance. Ensure that balance.formatted can be safely converted to a number, and consider using parseFloat for clarity.
141-145:
Correct reactive statement to ensure checkBalance() is triggered appropriately
Similarly, the reactive statement for checkBalance() may not trigger as intended. Directly referencing the variables establishes the necessary reactivity.
Update the reactive statement as follows:
-$: {
- if (account && network && paymentCurrencies[0]) {
- checkBalance();
- }
-}
+$: if (account && network && paymentCurrencies[0]) {
+ checkBalance();
+}This adjustment ensures that checkBalance() is called whenever account, network, or paymentCurrencies[0] changes.
Likely invalid or redundant comment.
packages/invoice-dashboard/src/lib/view-requests.svelte (3)
48-48: LGTM!
The import statement is correctly formatted.
55-60: LGTM!
The variable initializations are correctly typed and formatted for improved readability.
447-452: LGTM!
The Switch component is correctly implemented with proper binding and clear labeling.
…1528-bug-in-invoicing-template-wallet-list-loading-and-payment-failure
…1528-bug-in-invoicing-template-wallet-list-loading-and-payment-failure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (4)
482-486: Add error handling for invalid number inputsThe function should handle edge cases like NaN or Infinity to prevent potential display issues.
function formatBalance(value: number, maxDecimals: number = 4): string { + if (!Number.isFinite(value)) { + return '0'; + } return Number.isInteger(value) ? value.toString() : value.toFixed(maxDecimals); }
488-530: Enhance error logging in checkBalance functionThe error handling is good, but the error logging could be more specific to help with debugging.
} catch (err) { - console.error("Error checking balance:", err); + console.error("Error checking balance:", { + error: err, + context: { + address, + network, + paymentCurrency: paymentCurrencies[0]?.symbol, + chainId: account.chainId + } + }); hasEnoughBalance = false; userBalance = "0"; }
841-862: Enhance the insufficient balance warning messageThe warning message could be more informative by showing the required amount.
<div class="balance-warning"> - Insufficient funds: {userBalance} - {paymentCurrencies[0]?.symbol || "ETH"} + Insufficient funds: {userBalance} {paymentCurrencies[0]?.symbol || "ETH"} available + (Required: {formatUnits(request.expectedAmount, currency?.decimals ?? 18)} {paymentCurrencies[0]?.symbol || "ETH"}) </div>
1231-1244: Adjust disabled button opacity for better visual feedbackConsider reducing the opacity of disabled buttons for a clearer visual indication.
:global(.invoice-view-actions button[disabled]) { - opacity: 0.5; + opacity: 0.4; cursor: not-allowed; background-color: #71717a !important; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(8 hunks)
🔇 Additional comments (2)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
94-95: LGTM: Reactive state management for balance checking
The reactive statement and variables are well-structured, with proper initialization and dependencies.
Also applies to: 157-161
488-530: Verify integration with wallet connection flow
The balance checking implementation looks good, but let's verify its integration with the wallet connection flow.
✅ Verification successful
Balance check is properly integrated with wallet connection flow
The implementation is correctly integrated with the wallet connection flow based on the following evidence:
- The balance check is reactive to account changes:
$: { if (account && network) { checkBalance(); } } - The balance check properly validates the wallet connection state by checking
addressavailability - Network validation is handled correctly before balance checks
- The function is integrated with the wallet's chain switching mechanism
- Error handling is implemented for both network and balance check failures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for wallet connection handling code
rg -A 5 "wallet|connect" --type svelte
Length of output: 71
Script:
#!/bin/bash
# Search for wallet connection handling code using ripgrep without file type filter
rg -A 5 "wallet|connect" packages/invoice-dashboard/src
Length of output: 3481
Script:
#!/bin/bash
# Let's check the account and wallet initialization in the invoice view
rg -B 5 -A 10 "account|signer" packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Length of output: 6319
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/create-invoice-form/src/lib/create-invoice-form.svelte (1)
Line range hint
144-156: Add wallet connection and network validation.To prevent wallet-related issues during invoice creation, consider adding explicit validations for wallet connection status and network matching.
Enhance the validation logic:
$: { const basicDetailsFilled = formData.payeeAddress && formData.payerAddress && formData.dueDate && formData.invoiceNumber && formData.issuedOn && invoiceCurrency && currency && formData.issuedOn; const hasItems = formData.invoiceItems.length > 0 && formData.invoiceItems.every(isValidItem); + const isWalletReady = account?.isConnected && + account?.chain?.id === getChainIdFromNetwork(network); - canSubmit = basicDetailsFilled && hasItems && requestNetwork ? true : false; + canSubmit = basicDetailsFilled && hasItems && requestNetwork && isWalletReady; }Add a helper function to map network names to chain IDs:
function getChainIdFromNetwork(network?: string): number | undefined { if (!network) return undefined; // Add mapping based on your supported networks const networkToChainId: Record<string, number> = { 'mainnet': 1, 'goerli': 5, // Add other networks as needed }; return networkToChainId[network]; }
🧹 Nitpick comments (2)
shared/components/status-label.svelte (1)
Line range hint
8-32: Consider adding error handling for invalid status values.The switch statement currently defaults to "status-created" for any unrecognized status. While this works as a fallback, it might mask potential issues where invalid status values are being passed.
Consider this improvement:
$: { + const validStatus = status?.toLowerCase() ?? ''; - switch (status.toLowerCase()) { + switch (validStatus) { case "paid": statusClass = "status-paid"; break; // ... existing cases ... default: + console.warn(`Unexpected status value: ${status}`); statusClass = "status-created"; } }packages/create-invoice-form/src/lib/create-invoice-form.svelte (1)
Line range hint
213-227: Enhance error messaging for wallet-related issues.Given the PR objectives to fix wallet loading and payment processing issues, consider adding more specific error messages for wallet-related failures.
Add specific error handling for common wallet issues:
} catch (error: any) { if (error.message.includes("Transaction confirmation not received")) { isTimeout = true; removeAllStatuses(); + } else if (error.message.includes("insufficient funds")) { + addToStatus(APP_STATUS.ERROR_OCCURRED); + console.error("Insufficient funds in wallet:", error); + } else if (error.message.includes("user rejected")) { + addToStatus(APP_STATUS.ERROR_OCCURRED); + console.error("Transaction rejected by user:", error); } else { addToStatus(APP_STATUS.ERROR_OCCURRED); console.error("Failed to create request:", error); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/create-invoice-form/src/lib/create-invoice-form.svelte(1 hunks)packages/create-invoice-form/src/lib/invoice/form-view.svelte(2 hunks)packages/create-invoice-form/src/lib/invoice/form.svelte(2 hunks)packages/create-invoice-form/src/lib/utils/resetForm.ts(1 hunks)packages/invoice-dashboard/src/lib/view-requests.svelte(2 hunks)shared/components/status-label.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/create-invoice-form/src/lib/invoice/form-view.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/invoice-dashboard/src/lib/view-requests.svelte
🔇 Additional comments (6)
packages/create-invoice-form/src/lib/utils/resetForm.ts (1)
9-9: LGTM! Good separation of concerns.
The initialization of issuedOn as an empty string allows the form component to handle the date initialization logic through reactive statements, making the code more maintainable.
packages/create-invoice-form/src/lib/invoice/form.svelte (2)
179-187: LGTM! Good defensive programming with date initialization.
The reactive statements ensure that:
- Issue date defaults to current date when empty
- Due date defaults to one day after issue date when empty
This prevents invalid date states and improves form reliability.
598-598: LGTM! Good UX improvement for date selection.
The min attribute on the due date input prevents selecting a due date before the issue date, which is a good validation constraint.
Also applies to: 605-606
shared/components/status-label.svelte (2)
46-47: LGTM! Layout improvements enhance readability.
The changes from text-align: center to display: inline-flex with align-items: center provide better content alignment. The addition of white-space: nowrap and font-size: 14px ensures consistent text presentation without unwanted wrapping.
Also applies to: 53-54
46-54: Verify impact on payment status visibility.
While these styling improvements enhance the display of status labels, please ensure they work well with all possible payment states, particularly during the payment processing scenarios that this PR aims to fix.
packages/create-invoice-form/src/lib/create-invoice-form.svelte (1)
118-118: LGTM! Verify encryption functionality.
The reactive initialization of cipherProvider from requestNetwork?.getCipherProvider() is a good improvement, safely handling cases where requestNetwork is undefined.
Let's verify the encryption-related functionality:
✅ Verification successful
Encryption functionality verified and working correctly
The codebase analysis confirms that the cipherProvider initialization change is safe and properly integrated with the encryption functionality:
- The
cipherProvideris correctly typed asCipherProviderTypes.ICipherProvider - It's properly used in the form component for encryption-related features:
- Enables/disables the "Encrypt invoice" checkbox based on provider availability
- Used in
submitFormwith proper encryption parameters for both payee and payer
- The dashboard component also correctly uses the provider for decryption functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for encryption-related code and potential issues
# Check for encryption-related code patterns
echo "Checking encryption-related patterns..."
rg -A 5 "isEncrypted|EncryptionTypes|cipherProvider" --type svelte
# Check for potential encryption configuration
echo "Checking encryption configuration..."
fd -e json -e js -e ts "config|settings" -x rg -l "encryption|cipher"
Length of output: 343
Script:
#!/bin/bash
# Let's try a different approach to find encryption-related code
# Search for cipherProvider usage
echo "Searching for cipherProvider usage..."
rg "cipherProvider" -A 5
# Search for encryption-related imports and types
echo "Searching for encryption-related imports and types..."
rg "import.*CipherProvider|type.*CipherProvider" -A 2
# Search for encryption-related function calls
echo "Searching for encryption-related function calls..."
rg "encrypt|decrypt" -A 3
Length of output: 9674
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Outdated
Show resolved
Hide resolved
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Outdated
Show resolved
Hide resolved
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Outdated
Show resolved
Hide resolved
…1528-bug-in-invoicing-template-wallet-list-loading-and-payment-failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
157-161: Improve reactive statement specificity.The current reactive statement might run unnecessarily. Make it more specific about when the balance check should occur.
-$: { - if (account && network) { - checkBalance(); - } -} +$: if (account?.address && network && !isPaid && !isPayee) { + checkBalance(); +}
Line range hint
534-562: Enhance error handling in payment process.While the payment flow is correct, the error handling could be more specific to help users understand and resolve issues.
async function handlePayment() { try { await switchNetworkIfNeeded(network || "mainnet"); if ( !approved && paymentCurrencies[0]?.type === Types.RequestLogic.CURRENCY.ERC20 ) { await approve(); const approveStatus = statuses.find((s) => s.name === "APPROVE_ERC20"); if (approveStatus) approveStatus.done = true; return; } await payTheRequest(); } catch (err) { - console.error("Error during payment process:", err); - toast.error("Payment process failed", { - description: String(err), - }); + const errorMessage = err instanceof Error ? err.message : String(err); + if (errorMessage.includes("insufficient funds")) { + toast.error("Insufficient funds", { + description: "Please ensure you have enough balance to cover the payment and gas fees", + }); + } else if (errorMessage.includes("user rejected")) { + toast.error("Transaction cancelled", { + description: "You rejected the transaction", + }); + } else { + toast.error("Payment process failed", { + description: errorMessage, + }); + } statuses = statuses.map((status) => ({ ...status, done: false, })); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(8 hunks)shared/utils/initCurrencyManager.ts(2 hunks)
🔇 Additional comments (4)
shared/utils/initCurrencyManager.ts (1)
65-65: LGTM! Code formatting improvements enhance readability.
The changes improve code readability by:
- Consistent use of double quotes for imports
- Multi-line formatting for function parameters and return types
- Better alignment of CurrencyManager instantiation
Also applies to: 72-76, 84-88, 99-103, 108-115
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (3)
94-95: LGTM! New state variables for balance management.
The addition of these state variables helps track and display the user's balance status.
841-862: LGTM! Clear user feedback for insufficient balance.
The UI changes effectively communicate balance status to users:
- Warning message shows current balance
- Payment button is disabled when balance is insufficient
- Styles are consistent with the application theme
Also applies to: 1231-1244
482-486:
Use BigInt for balance formatting to prevent precision loss.
The current implementation uses Number which could lead to precision loss with large amounts.
-function formatBalance(value: number, maxDecimals: number = 4): string {
- return Number.isInteger(value)
- ? value.toString()
- : value.toFixed(maxDecimals);
+function formatBalance(value: bigint, maxDecimals: number = 4): string {
+ return value.toString();
}Likely invalid or redundant comment.
sstefdev
deleted the
1528-bug-in-invoicing-template-wallet-list-loading-and-payment-failure
branch
Fixes #50
Fixes RequestNetwork/requestNetwork#1528
Problem
The Invoicing Template encountered two significant issues that disrupt its functionality:
Changes
Note
Could not reproduce the payment error
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores