feat!: Override Parameter for Currencies and Payment Chains in Invoice Dashboard and Create Invoice Form #242
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, focusing on improving the handling of currency management, network icons, and cipher providers. Key changes include updating the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (4)packages/create-invoice-form/src/lib/create-invoice-form.svelte (1)
The reactive statement safely initializes cipherProvider using optional chaining. packages/invoice-dashboard/src/lib/view-requests.svelte (3)
The explicit type definition and assertion provides better type safety by ensuring the cipherProvider has the required
Using the nullish coalescing operator to provide a "-" fallback improves the handling of undefined currency symbols.
The current implementation triggers enableDecryption directly from the reactive statement which could lead to unnecessary calls. As suggested in the previous review, consider implementing debouncing: - $: sliderValueForDecryption, enableDecryption();
+ $: {
+ try {
+ const isEnabled = sliderValueForDecryption === 'on';
+ enableDecryption(isEnabled);
+ // Debounce request refresh to prevent unnecessary calls
+ debouncedUpdate(() => getRequests());
+ } catch (error) {
+ console.error('Failed to toggle decryption:', error);
+ toast.error('Failed to toggle decryption');
+ }
+ }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 (
|
sstefdev
changed the title
sstefdev
requested review from
MantisClone,
aimensahnoun and
rodrigopavezi
and removed request for
MantisClone and
rodrigopavezi
…parameter-_override_-the-default-currency-and-payment-chains-list-instead-of-append
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
shared/icons/network/network-icon.svelte (1)
13-13: LGTM! Consider i18n for the "Unknown" text.The addition of the "Unknown" fallback text improves user experience by providing clear feedback when a network name is not available.
Consider extracting "Unknown" to a translation constant for future internationalization support:
- <span>{capitalize(network) || "Unknown"}</span> + <span>{capitalize(network) || t('common.unknown')}</span>shared/utils/initCurrencyManager.ts (1)
75-80: Consider memoizing default currencies initializationThe default currencies filtering could be expensive if called frequently.
+const memoizedDefaultCurrencies = new Map(); + export function initializeCurrencyManager( customCurrencies: CurrencyTypes.CurrencyInput[] = [] ): CurrencyManager { if (customCurrencies?.length > 0) { return new CurrencyManager(customCurrencies, {}, formattedCurrencyConversionPairs); } + const cacheKey = defaultCurrencyIds.join(','); + if (!memoizedDefaultCurrencies.has(cacheKey)) { + const defaultCurrencies = CurrencyManager.getDefaultList().filter( + (currency) => defaultCurrencyIds.includes(currency.id) + ); + memoizedDefaultCurrencies.set(cacheKey, defaultCurrencies); + } + - const defaultCurrencies = CurrencyManager.getDefaultList().filter( - (currency) => defaultCurrencyIds.includes(currency.id) - ); - - return new CurrencyManager(defaultCurrencies, {}, formattedCurrencyConversionPairs); + return new CurrencyManager( + memoizedDefaultCurrencies.get(cacheKey), + {}, + formattedCurrencyConversionPairs + );packages/invoice-dashboard/src/lib/view-requests.svelte (2)
332-332: LGTM! Consider using nullish coalescing operator.The fallback to "-" when currency symbol is undefined is good practice. Consider using the nullish coalescing operator for a more concise expression:
- currencySymbol: currencyInfo?.symbol ?? "-", + currencySymbol: currencyInfo?.symbol ?? "-",
332-332: Consider performance optimizations for table operations.The table implementation performs multiple array operations on each change. Consider these performance improvements:
- Memoize expensive computations:
- Use
memoizefor sorting and filtering operations- Cache processed requests until relevant data changes
- Reduce array copies:
- Use immutable array operations
- Consider using a virtual scroll implementation for large datasets
- Optimize reactive dependencies:
- Split large reactive statements into smaller, focused ones
- Use computed stores for derived data
Would you like me to provide specific code examples for any of these optimizations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/invoice-dashboard/src/lib/view-requests.svelte(1 hunks)shared/icons/network/network-icon.svelte(1 hunks)shared/icons/unkown.svelte(1 hunks)shared/utils/getNetworkIcon.ts(2 hunks)shared/utils/initCurrencyManager.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- shared/icons/unkown.svelte
🔇 Additional comments (1)
shared/utils/initCurrencyManager.ts (1)
70-80: Verify currency override behavior
Let's ensure the currency override functionality works as intended per PR objectives.
✅ Verification successful
Let me gather more specific information about the currency override functionality.
Let me check one more aspect to understand how the currency manager is being used in the application.
Currency override behavior is properly implemented
The implementation correctly handles both custom and default currency scenarios:
- When
customCurrenciesare provided, it exclusively uses those currencies - When no custom currencies are provided, it falls back to a filtered list of default currencies (USD, EUR)
- The initialization is properly encapsulated in the
initializeCurrencyManagerfunction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for currency override usage patterns
rg -A 5 "initializeCurrencyManager|CurrencyManager" --type ts --type svelte
# Check for any currency-related test files
fd -e test.ts -e spec.ts | xargs rg -l "currency|currencies"
Length of output: 1868
Script:
#!/bin/bash
# Check the CurrencyManager class implementation and usage
ast-grep --pattern 'class CurrencyManager {
$$$
}'
# Look for tests specifically related to currency manager initialization
rg -B2 -A5 "initCurrencyManager" shared/utils/initCurrencyManager.ts
# Check for customCurrencies usage in the codebase
rg "customCurrencies" --type ts -B2 -A2
Length of output: 791
Script:
#!/bin/bash
# Check how CurrencyManager.getDefaultList is implemented and used
ast-grep --pattern 'getDefaultList() {
$$$
}'
# Look for actual usage of initializeCurrencyManager in the codebase
rg "initializeCurrencyManager\(" --type ts -B2 -A2
# Check the defaultCurrencyIds usage
rg "defaultCurrencyIds" --type ts -B2 -A2
Length of output: 1160
…209-make-currencies-and-payment-chain-input-parameter-_override_-the-default-currency-and-payment-chains-list-instead-of-append
…parameter-_override_-the-default-currency-and-payment-chains-list-instead-of-append
…override_-the-default-currency-and-payment-chains-list-instead-of-append' of github.com:RequestNetwork/web-components into 209-make-currencies-and-payment-chain-input-parameter-_override_-the-default-currency-and-payment-chains-list-instead-of-append
There was a problem hiding this comment.
@sstefdev Please update the description with usage examples for both the append and override behavior.
As far as I can tell - the description is incorrect. It mentions a new "append or override" parameter, but this isn't present in the code.
…parameter-_override_-the-default-currency-and-payment-chains-list-instead-of-append
…209-make-currencies-and-payment-chain-input-parameter-_override_-the-default-currency-and-payment-chains-list-instead-of-append
…override_-the-default-currency-and-payment-chains-list-instead-of-append' of github.com:RequestNetwork/web-components into 209-make-currencies-and-payment-chain-input-parameter-_override_-the-default-currency-and-payment-chains-list-instead-of-append
MantisClone
changed the title
|
I added |
|
I edited the description. @sstefdev please review my edits before merging. |
sstefdev
deleted the
209-make-currencies-and-payment-chain-input-parameter-_override_-the-default-currency-and-payment-chains-list-instead-of-append
branch
Fixes #209
Problem
Currently, the currencies input parameter appends to the default currency list in the Invoice Dashboard and Create Invoice Form. This makes it impossible for app builders to remove a currency from the default list, limiting customization options.
Changes
BREAKING CHANGE!
currenciesparameter to act like an override for the default currency list instead of appending to the currency list on both the Invoice Dashboard and Create Invoice Form components.currenciesparameter is passed, only those currencies.currenciesparameter is not passed, the default currencies list is used.Considerations
App Builders who previously used the
currenciesparameter to append to the default currency list must now treat it like an override by including every currency they wish to support in their currencies argument instead of only the appended currencies.How it looks when we don't have the right currencies in the config, but we have them in the invocies
Summary by CodeRabbit
New Features
Bug Fixes
Chores