Fees redesign #16702
Fees redesign #16702
Conversation
enjojoy
force-pushed
the
fees-redesign
branch
7 times, most recently
from
edfd48e to
da997b2
Compare
enjojoy
force-pushed
the
fees-redesign
branch
from
da997b2 to
bef5516
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
enjojoy
force-pushed
the
fees-redesign
branch
2 times, most recently
from
1434764 to
2da9429
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
enjojoy
force-pushed
the
fees-redesign
branch
from
2da9429 to
c8a07e8
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
packages/components/src/components/form/SelectBar/SelectBar.tsx
Outdated
Show resolved
Hide resolved
enjojoy
force-pushed
the
fees-redesign
branch
from
c8a07e8 to
fb83977
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
enjojoy
force-pushed
the
fees-redesign
branch
from
46a5401 to
fb9d05f
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
enjojoy
force-pushed
the
fees-redesign
branch
from
fb9d05f to
3b55ee1
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
enjojoy
force-pushed
the
fees-redesign
branch
2 times, most recently
from
54e56af to
25675a3
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
enjojoy
force-pushed
the
fees-redesign
branch
from
25675a3 to
1e4a8c2
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
enjojoy
force-pushed
the
fees-redesign
branch
from
1e4a8c2 to
2f3f2bf
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
enjojoy
force-pushed
the
fees-redesign
branch
from
2f3f2bf to
3b6015f
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
enjojoy
force-pushed
the
fees-redesign
branch
from
3b6015f to
63093d1
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
enjojoy
force-pushed
the
fees-redesign
branch
from
63093d1 to
598a42e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/suite/src/components/wallet/Fees/CustomFee.tsx (1)
53-53:⚠️ Potential issueAdd safety checks and proper average calculation.
The current implementation has several issues:
- Missing input validation for null/undefined/empty arrays
- Not handling missing feePerUnit property
- Not calculating a true average (sum of items / number of items)
Apply this diff to fix the issues:
-const getAverageFee = (levels: FeeLevel[]) => `${levels[levels.length > 2 ? 1 : 0].feePerUnit}`; +const getAverageFee = (levels: FeeLevel[]): string => { + if (!levels?.length) { + return '0'; + } + const validLevels = levels.filter(level => level.feePerUnit !== undefined); + if (!validLevels.length) { + return '0'; + } + const sum = validLevels.reduce((acc, level) => acc + Number(level.feePerUnit), 0); + return String(sum / validLevels.length); +};packages/suite/src/components/wallet/Fees/FeeDetails.tsx (3)
171-198: 🛠️ Refactor suggestionImprove robustness of fee options mapping.
The same improvements needed as in BitcoinDetails component:
- Use optional chaining for safer array iteration
- Use a more stable key (fee.value) instead of array index
- Use nullish coalescing (
??) instead of logical OR (||) for proper fallback handlingApply this diff:
- {feeOptions && - feeOptions.map((fee, index) => ( + {feeOptions?.map(fee => ( <FeeCard - key={index} + key={fee.value} value={fee.value} setSelectedLevelOption={setSelectedLevelOption} isSelected={selectedLevelOption === fee.value} changeFeeLevel={changeFeeLevel} topLeftChild={ <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> } bottomLeftChild={ <FiatValue disableHiddenPlaceholder - amount={fee.networkAmount || ''} + amount={fee.networkAmount ?? ''} symbol={symbol} /> }🧰 Tools
🪛 Biome (1.9.4)
[error] 171-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
206-234:⚠️ Potential issueAdd validation for empty feeOptions array.
The component assumes feeOptions[0] exists, which could cause runtime errors if the array is empty.
Apply this diff:
-const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => +const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => showFee && ( + feeOptions.length > 0 && ( <Column padding={spacings.xxxs} width="100%"> <FeeCard value={feeOptions[0].value}
111-139: 🛠️ Refactor suggestionImprove robustness of fee options mapping.
Several improvements can enhance the component's reliability:
- Use optional chaining for safer array iteration
- Use a more stable key (fee.value) instead of array index
- Use nullish coalescing (
??) instead of logical OR (||) for proper fallback handlingApply this diff:
- {feeOptions && - feeOptions.map((fee, index) => ( + {feeOptions?.map(fee => ( <FeeCard - key={index} + key={fee.value} value={fee.value} setSelectedLevelOption={setSelectedLevelOption} isSelected={selectedLevelOption === fee.value} changeFeeLevel={changeFeeLevel} topLeftChild={ <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> } topRightChild={ - <>~{formatDuration(feeInfo.blockTime * (fee?.blocks || 0) * 60)}</> + <>~{formatDuration(feeInfo.blockTime * (fee?.blocks ?? 0) * 60)}</> } bottomLeftChild={ <FiatValue disableHiddenPlaceholder - amount={fee?.networkAmount || ''} + amount={fee?.networkAmount ?? ''} symbol={symbol} /> }🧰 Tools
🪛 Biome (1.9.4)
[error] 111-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/suite/src/components/wallet/Fees/Fees.tsx (1)
285-313: 🛠️ Refactor suggestionAdd detailed fee breakdown for Ethereum transactions.
The current implementation only shows the total fee amount without breaking down gas price and gas limit for Ethereum transactions, which reduces transparency for users.
Apply this diff to add the breakdown:
{networkAmount && ( <Column> <Divider margin={{ bottom: spacings.md }} /> + {networkType === 'ethereum' && ( + <> + <Row + gap={spacings.sm} + alignItems="baseline" + justifyContent="space-between" + margin={{ bottom: spacings.xs }} + > + <Text variant="tertiary"> + <Translation id="TR_GAS_PRICE" />: + </Text> + <Text> + {selectedLevel.feePerUnit} {getFeeUnits(networkType)} + </Text> + </Row> + <Row + gap={spacings.sm} + alignItems="baseline" + justifyContent="space-between" + margin={{ bottom: spacings.xs }} + > + <Text variant="tertiary"> + <Translation id="TR_GAS_LIMIT" />: + </Text> + <Text>{getValues('feeLimit')}</Text> + </Row> + </> + )} <Row gap={spacings.sm} alignItems="baseline" justifyContent="space-between" > <Text variant="tertiary"> - <Translation id="FEE" />: + <Translation + id={networkType === 'ethereum' ? 'TR_TOTAL_FEE' : 'FEE'} + />: </Text>
🧹 Nitpick comments (1)
packages/suite/src/components/wallet/Fees/Fees.tsx (1)
98-159: Refactor to reduce code duplication in network amount formatting.The network amount formatting logic is duplicated across different network type handlers.
Apply this diff to extract the common logic:
+ const formatFeeOption = ( + level: FeeLevel, + transactionInfo?: PrecomposedLevels | PrecomposedLevelsCardano, + extraFields?: Partial<FeeOption> + ): FeeOption => { + const hasTransactionInfo = + transactionInfo?.[level.label] !== undefined && + transactionInfo[level.label].type !== 'error'; + const networkAmount = hasTransactionInfo + ? formatNetworkAmount(transactionInfo[level.label].fee, symbol) + : null; + + return { + label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), + value: level.label, + feePerUnit: level.feePerUnit, + networkAmount, + ...extraFields, + }; + }; const buildFeeOptions = ( levels: FeeLevel[], networkType: NetworkType, symbol: NetworkSymbol, composedLevels?: PrecomposedLevels | PrecomposedLevelsCardano, ) => { const filteredLevels = levels.filter(level => level.label !== 'custom'); if (networkType === 'ethereum') { - return filteredLevels.map(level => { - const transactionInfo = composedLevels?.[level.label]; - const hasTransactionInfo = - transactionInfo !== undefined && transactionInfo.type !== 'error'; - const networkAmount = hasTransactionInfo - ? formatNetworkAmount(transactionInfo.fee, symbol) - : null; - - return { - label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), - value: level.label, - feePerUnit: level.feePerUnit, - networkAmount, - }; - }); + return filteredLevels.map(level => formatFeeOption(level, composedLevels)); } if (networkType === 'bitcoin') { - return filteredLevels.map(level => { - const transactionInfo = composedLevels?.[level.label]; - const hasTransactionInfo = - transactionInfo !== undefined && transactionInfo.type !== 'error'; - const networkAmount = hasTransactionInfo - ? formatNetworkAmount(transactionInfo.fee, symbol) - : null; - - return { - label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), - value: level.label, - blocks: level.blocks, - feePerUnit: level.feePerUnit, - networkAmount, - }; - }); + return filteredLevels.map(level => + formatFeeOption(level, composedLevels, { blocks: level.blocks }) + ); } - return filteredLevels.map(level => { - const transactionInfo = composedLevels?.[level.label]; - const hasTransactionInfo = - transactionInfo !== undefined && transactionInfo.type !== 'error'; - const networkAmount = hasTransactionInfo - ? formatNetworkAmount(transactionInfo.fee, symbol) - : null; - - return { - label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), - value: level.label, - feePerUnit: level.feePerUnit, - networkAmount, - }; - }); + return filteredLevels.map(level => formatFeeOption(level, composedLevels)); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/suite/src/components/wallet/Fees/CustomFee.tsx(4 hunks)packages/suite/src/components/wallet/Fees/FeeDetails.tsx(1 hunks)packages/suite/src/components/wallet/Fees/Fees.tsx(6 hunks)packages/suite/src/hooks/wallet/__fixtures__/useSendForm.ts(3 hunks)packages/suite/src/support/messages.ts(3 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInputs.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInputs.tsx
- packages/suite/src/hooks/wallet/fixtures/useSendForm.ts
- packages/suite/src/support/messages.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/components/wallet/Fees/FeeDetails.tsx
[error] 111-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 171-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
🔇 Additional comments (4)
packages/suite/src/components/wallet/Fees/CustomFee.tsx (2)
174-189: LGTM!The Row component implementation is well-structured with proper spacing and alignment between the fee display and icon.
160-160: LGTM!The Column component uses consistent spacing values from the theme.
packages/suite/src/components/wallet/Fees/FeeDetails.tsx (1)
45-48: LGTM!The styled component provides a clean and simple grid layout.
packages/suite/src/components/wallet/Fees/Fees.tsx (1)
229-316: LGTM!The animation implementation is well-structured with proper easing and timing for smooth transitions between fee views.
enjojoy
force-pushed
the
fees-redesign
branch
from
598a42e to
77866e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
packages/suite/src/components/wallet/Fees/CustomFee.tsx (3)
53-53:⚠️ Potential issueAdd safety checks and proper average calculation.
The function needs additional safety checks and proper average calculation:
- It doesn't handle null/undefined inputs or empty arrays.
- It doesn't handle missing feePerUnit property.
- It doesn't calculate a true average (sum of items / number of items).
Apply this diff to fix the issues:
-const getAverageFee = (levels: FeeLevel[]) => `${levels[levels.length > 2 ? 1 : 0].feePerUnit}`; +const getAverageFee = (levels: FeeLevel[]): string => { + if (!levels?.length) { + return '0'; + } + const validLevels = levels.filter(level => level.feePerUnit !== undefined); + if (!validLevels.length) { + return '0'; + } + const sum = validLevels.reduce((acc, level) => acc + Number(level.feePerUnit), 0); + return String(sum / validLevels.length); +};
53-53:⚠️ Potential issueImprove robustness of getAverageFee function.
The function needs proper validation and true average calculation:
- No validation for null/undefined/empty arrays
- No validation for missing feePerUnit property
- Not calculating a true average (sum/count)
Apply this diff to fix the issues:
-const getAverageFee = (levels: FeeLevel[]) => `${levels[levels.length > 2 ? 1 : 0].feePerUnit}`; +const getAverageFee = (levels: FeeLevel[]): string => { + if (!levels?.length) return '0'; + const validLevels = levels.filter(level => level.feePerUnit !== undefined); + if (!validLevels.length) return '0'; + const sum = validLevels.reduce((acc, level) => acc + Number(level.feePerUnit), 0); + return String(Math.round(sum / validLevels.length)); +};
53-53:⚠️ Potential issueImprove robustness of getAverageFee function.
The current implementation has potential issues:
- No validation for null/undefined inputs or empty arrays
- Direct array indexing without bounds checking
- Not calculating a true average (sum/count)
Apply this diff to fix the issues:
-const getAverageFee = (levels: FeeLevel[]) => `${levels[levels.length > 2 ? 1 : 0].feePerUnit}`; +const getAverageFee = (levels: FeeLevel[]): string => { + if (!levels?.length) return '0'; + const validLevels = levels.filter(level => level.feePerUnit !== undefined); + if (!validLevels.length) return '0'; + const sum = validLevels.reduce((acc, level) => acc + Number(level.feePerUnit), 0); + return String(Math.round(sum / validLevels.length)); +};packages/suite/src/components/wallet/Fees/FeeDetails.tsx (5)
206-234:⚠️ Potential issueAdd validation for empty feeOptions array.
The MiscDetails component assumes feeOptions[0] exists, which could cause runtime errors if the array is empty.
Apply this diff:
-const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => +const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => showFee && ( + feeOptions.length > 0 && ( <Column padding={spacings.xxxs} width="100%"> <FeeCard value={feeOptions[0].value}
111-139: 🛠️ Refactor suggestionImprove robustness of BitcoinDetails component.
Several improvements can enhance the component's reliability:
- Replace logical OR (
||) with nullish coalescing (??) for proper fallback handling.- Use a more stable key for the map function instead of array index.
- Add null checks for fee.blocks.
Apply this diff:
- {feeOptions && - feeOptions.map((fee, index) => ( + {feeOptions?.map((fee) => ( <FeeCard - key={index} + key={fee.value} value={fee.value} setSelectedLevelOption={setSelectedLevelOption} isSelected={selectedLevelOption === fee.value} changeFeeLevel={changeFeeLevel} topLeftChild={ <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> } topRightChild={ - <>~{formatDuration(feeInfo.blockTime * (fee?.blocks || 0) * 60)}</> + <>~{formatDuration(feeInfo.blockTime * (fee?.blocks ?? 0) * 60)}</> } bottomLeftChild={ <FiatValue disableHiddenPlaceholder - amount={fee?.networkAmount || ''} + amount={fee?.networkAmount ?? ''} symbol={symbol} /> }🧰 Tools
🪛 Biome (1.9.4)
[error] 111-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
111-139: 🛠️ Refactor suggestionImprove robustness of BitcoinDetails component.
Several improvements needed:
- Array index used as key could cause issues with React reconciliation
- Missing optional chaining for safer property access
- Using logical OR (
||) instead of nullish coalescing (??)Apply this diff:
- {feeOptions && - feeOptions.map((fee, index) => ( + {feeOptions?.map((fee) => ( <FeeCard - key={index} + key={fee.value} value={fee.value} setSelectedLevelOption={setSelectedLevelOption} isSelected={selectedLevelOption === fee.value} changeFeeLevel={changeFeeLevel} topLeftChild={ <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> } topRightChild={ - <>~{formatDuration(feeInfo.blockTime * (fee?.blocks || 0) * 60)}</> + <>~{formatDuration(feeInfo.blockTime * (fee?.blocks ?? 0) * 60)}</> } bottomLeftChild={ <FiatValue disableHiddenPlaceholder - amount={fee?.networkAmount || ''} + amount={fee?.networkAmount ?? ''} symbol={symbol} /> }🧰 Tools
🪛 Biome (1.9.4)
[error] 111-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
206-234:⚠️ Potential issueAdd validation for MiscDetails component.
The component assumes feeOptions[0] exists without validation.
Apply this diff:
-const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => +const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => showFee && ( + feeOptions?.length > 0 && ( <Column padding={spacings.xxxs} width="100%"> <FeeCard value={feeOptions[0].value} setSelectedLevelOption={() => {}} isSelected={true} changeFeeLevel={() => {}} topLeftChild={ <span data-testid={`fee-card/${feeOptions[0].value}`}> {feeOptions[0].label} </span> } topRightChild="" bottomLeftChild={ <FiatValue disableHiddenPlaceholder - amount={feeOptions[0].networkAmount || ''} + amount={feeOptions[0].networkAmount ?? ''} symbol={symbol} /> }
111-139: 🛠️ Refactor suggestionImprove robustness of BitcoinDetails component.
Several improvements can enhance the component's reliability:
- Replace logical OR (
||) with nullish coalescing (??) for proper fallback handling- Use a more stable key for the map function instead of array index
- Add null checks for fee.blocks
Apply this diff:
- {feeOptions && - feeOptions.map((fee, index) => ( + {feeOptions?.map((fee) => ( <FeeCard - key={index} + key={fee.value} value={fee.value} setSelectedLevelOption={setSelectedLevelOption} isSelected={selectedLevelOption === fee.value} changeFeeLevel={changeFeeLevel} topLeftChild={ <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> } topRightChild={ - <>~{formatDuration(feeInfo.blockTime * (fee?.blocks || 0) * 60)}</> + <>~{formatDuration(feeInfo.blockTime * (fee?.blocks ?? 0) * 60)}</> } bottomLeftChild={ <FiatValue disableHiddenPlaceholder - amount={fee?.networkAmount || ''} + amount={fee?.networkAmount ?? ''} symbol={symbol} /> }🧰 Tools
🪛 Biome (1.9.4)
[error] 111-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🧹 Nitpick comments (3)
packages/suite/src/components/wallet/Fees/Fees.tsx (2)
85-147: Simplify fee options building logic to reduce duplication.The function has repeated logic for formatting network amounts and building fee options across different network types.
Consider this refactoring to reduce duplication:
const buildFeeOptions = ( levels: FeeLevel[], networkType: NetworkType, symbol: NetworkSymbol, translationString: TranslationFunction, composedLevels?: PrecomposedLevels | PrecomposedLevelsCardano, ) => { const filteredLevels = levels.filter(level => level.label !== 'custom'); + + const getNetworkAmount = (level: FeeLevel) => { + const transactionInfo = composedLevels?.[level.label]; + const hasTransactionInfo = + transactionInfo !== undefined && transactionInfo.type !== 'error'; + return hasTransactionInfo + ? formatNetworkAmount(transactionInfo.fee, symbol) + : null; + }; + + const baseOption = (level: FeeLevel) => ({ + label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), + value: level.label, + feePerUnit: level.feePerUnit, + networkAmount: getNetworkAmount(level), + }); - if (networkType === 'ethereum') { - return filteredLevels.map(level => { - const transactionInfo = composedLevels?.[level.label]; - const hasTransactionInfo = - transactionInfo !== undefined && transactionInfo.type !== 'error'; - const networkAmount = hasTransactionInfo - ? formatNetworkAmount(transactionInfo.fee, symbol) - : null; - - return { - label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), - value: level.label, - feePerUnit: level.feePerUnit, - networkAmount, - }; - }); - } - - if (networkType === 'bitcoin') { - return filteredLevels.map(level => { - const transactionInfo = composedLevels?.[level.label]; - const hasTransactionInfo = - transactionInfo !== undefined && transactionInfo.type !== 'error'; - const networkAmount = hasTransactionInfo - ? formatNetworkAmount(transactionInfo.fee, symbol) - : null; - - return { - label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), - value: level.label, - blocks: level.blocks, - feePerUnit: level.feePerUnit, - networkAmount, - }; - }); - } - - return filteredLevels.map(level => { - const transactionInfo = composedLevels?.[level.label]; - const hasTransactionInfo = - transactionInfo !== undefined && transactionInfo.type !== 'error'; - const networkAmount = hasTransactionInfo - ? formatNetworkAmount(transactionInfo.fee, symbol) - : null; - - return { - label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), - value: level.label, - feePerUnit: level.feePerUnit, - networkAmount, - }; - }); + return filteredLevels.map(level => ({ + ...baseOption(level), + ...(networkType === 'bitcoin' ? { blocks: level.blocks } : {}), + })); };
187-187: Move network capability check to a constant or utility function.The hardcoded check for Solana's custom fee support should be moved to a more maintainable location.
Consider moving this check to a utility function or constant:
+const NETWORKS_WITH_CUSTOM_FEES: NetworkType[] = ['bitcoin', 'ethereum']; +const supportsCustomFee = (networkType: NetworkType) => NETWORKS_WITH_CUSTOM_FEES.includes(networkType); + -const supportsCustomFee = networkType !== 'solana'; +const supportsCustomFee = supportsCustomFee(networkType);packages/suite/src/components/wallet/Fees/FeeDetails.tsx (1)
61-93: Consider memoizing FeeCard component for better performance.Since FeeCard is used in a mapping operation and receives multiple props, memoizing it could prevent unnecessary re-renders.
Apply this diff:
-const FeeCard = ({ +const FeeCard = React.memo(({ value, setSelectedLevelOption, isSelected, changeFeeLevel, topLeftChild, topRightChild, bottomLeftChild, bottomRightChild, -}: FeeCardProps) => ( +}: FeeCardProps) => { + const handleClick = React.useCallback(() => { + setSelectedLevelOption(value); + changeFeeLevel(value); + }, [value, setSelectedLevelOption, changeFeeLevel]); + + return ( <Box minWidth={170} margin={spacings.xxs}> <RadioCard - onClick={() => { - setSelectedLevelOption(value); - changeFeeLevel(value); - }} + onClick={handleClick} isActive={isSelected} > // ... rest of the component </RadioCard> </Box> -); + ); +}); + +FeeCard.displayName = 'FeeCard';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/suite/src/components/wallet/Fees/CustomFee.tsx(4 hunks)packages/suite/src/components/wallet/Fees/FeeDetails.tsx(1 hunks)packages/suite/src/components/wallet/Fees/Fees.tsx(7 hunks)packages/suite/src/hooks/wallet/__fixtures__/useSendForm.ts(3 hunks)packages/suite/src/support/messages.ts(3 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInputs.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInputs.tsx
- packages/suite/src/hooks/wallet/fixtures/useSendForm.ts
- packages/suite/src/support/messages.ts
👮 Files not reviewed due to content moderation or server errors (2)
- packages/suite/src/components/wallet/Fees/CustomFee.tsx
- packages/suite/src/components/wallet/Fees/FeeDetails.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/components/wallet/Fees/FeeDetails.tsx
[error] 111-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 171-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (11)
packages/suite/src/components/wallet/Fees/Fees.tsx (5)
56-62: LGTM! Well-structured type definition.The
FeeOptiontype is well-defined with clear, optional properties that accurately represent fee-related data.
64-68: LGTM! Styled component is properly placed.The
SelectBarWrapperis correctly defined at file level, ensuring it's not recreated on every render.
242-253: LGTM! Animation transitions are properly handled.The animation transitions use proper overflow handling to prevent UI jumping, addressing the previous concern.
328-334: LGTM! Error handling and helper text are well implemented.The error and helper text implementation follows good practices with proper spacing and components.
199-201: Fee display inconsistency for Ethereum network.The component uses different labels for Ethereum ('MAX_FEE') vs other networks ('FEE'), but doesn't show the gas price and gas limit details that users need.
packages/suite/src/components/wallet/Fees/CustomFee.tsx (3)
174-189: LGTM! UI changes enhance the fee display.The new Row component with Icon improves the visual representation of the current fee, aligning well with the fees interface redesign objectives.
160-189: LGTM! The UI changes improve clarity.The new Row component with Icon enhances the fee display by:
- Clearly showing the current fee with proper spacing
- Using contextual icons (gasPump for ethereum, receipt for others)
- Maintaining consistent typography and alignment
174-189: LGTM! Clean and well-structured UI for displaying current fee.The implementation uses proper typography components and clear layout structure.
packages/suite/src/components/wallet/Fees/FeeDetails.tsx (3)
239-248: LGTM! Good use of switch statement.The switch statement makes the component selection logic cleaner and more maintainable.
61-93: LGTM! Well-structured FeeCard component.The component is well-designed with:
- Clear props interface
- Proper event handling
- Consistent layout and spacing
- Reusable across different network types
239-248: LGTM! Clean switch statement for network-specific components.The implementation provides a clear and maintainable way to render different fee details based on network type.
| {feeOptions && | ||
| feeOptions.map((fee, index) => ( | ||
| <FeeCard | ||
| key={index} | ||
| value={fee.value} | ||
| setSelectedLevelOption={setSelectedLevelOption} | ||
| isSelected={selectedLevelOption === fee.value} | ||
| changeFeeLevel={changeFeeLevel} | ||
| topLeftChild={ | ||
| <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> | ||
| } | ||
| topRightChild="" | ||
| bottomLeftChild={ | ||
| <FiatValue | ||
| disableHiddenPlaceholder | ||
| amount={fee.networkAmount || ''} | ||
| symbol={symbol} | ||
| showApproximationIndicator | ||
| /> | ||
| } | ||
| bottomRightChild={ | ||
| <> | ||
| {formatFeePerUnit(fee?.feePerUnit)}{' '} | ||
| {getFeeUnits(networkType)} | ||
| </> | ||
| } | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply similar improvements to EthereumDetails component.
The same robustness improvements should be applied to the EthereumDetails component.
Apply this diff:
- {feeOptions &&
- feeOptions.map((fee, index) => (
+ {feeOptions?.map((fee) => (
<FeeCard
- key={index}
+ key={fee.value}
value={fee.value}
setSelectedLevelOption={setSelectedLevelOption}
isSelected={selectedLevelOption === fee.value}
changeFeeLevel={changeFeeLevel}
topLeftChild={
<span data-testid={`fee-card/${fee.value}`}>{fee.label}</span>
}
bottomLeftChild={
<FiatValue
disableHiddenPlaceholder
- amount={fee.networkAmount || ''}
+ amount={fee.networkAmount ?? ''}
symbol={symbol}
/>
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {feeOptions && | |
| feeOptions.map((fee, index) => ( | |
| <FeeCard | |
| key={index} | |
| value={fee.value} | |
| setSelectedLevelOption={setSelectedLevelOption} | |
| isSelected={selectedLevelOption === fee.value} | |
| changeFeeLevel={changeFeeLevel} | |
| topLeftChild={ | |
| <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> | |
| } | |
| topRightChild="" | |
| bottomLeftChild={ | |
| <FiatValue | |
| disableHiddenPlaceholder | |
| amount={fee.networkAmount || ''} | |
| symbol={symbol} | |
| showApproximationIndicator | |
| /> | |
| } | |
| bottomRightChild={ | |
| <> | |
| {formatFeePerUnit(fee?.feePerUnit)}{' '} | |
| {getFeeUnits(networkType)} | |
| </> | |
| } | |
| /> | |
| ))} | |
| {feeOptions?.map((fee) => ( | |
| <FeeCard | |
| key={fee.value} | |
| value={fee.value} | |
| setSelectedLevelOption={setSelectedLevelOption} | |
| isSelected={selectedLevelOption === fee.value} | |
| changeFeeLevel={changeFeeLevel} | |
| topLeftChild={ | |
| <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> | |
| } | |
| topRightChild="" | |
| bottomLeftChild={ | |
| <FiatValue | |
| disableHiddenPlaceholder | |
| amount={fee.networkAmount ?? ''} | |
| symbol={symbol} | |
| showApproximationIndicator | |
| /> | |
| } | |
| bottomRightChild={ | |
| <> | |
| {formatFeePerUnit(fee?.feePerUnit)}{' '} | |
| {getFeeUnits(networkType)} | |
| </> | |
| } | |
| /> | |
| ))} |
🧰 Tools
🪛 Biome (1.9.4)
[error] 171-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🛠️ Refactor suggestion
Apply same robustness improvements to EthereumDetails.
The same issues exist in the EthereumDetails component.
Apply this diff:
- {feeOptions &&
- feeOptions.map((fee, index) => (
+ {feeOptions?.map((fee) => (
<FeeCard
- key={index}
+ key={fee.value}
value={fee.value}
setSelectedLevelOption={setSelectedLevelOption}
isSelected={selectedLevelOption === fee.value}
changeFeeLevel={changeFeeLevel}
topLeftChild={
<span data-testid={`fee-card/${fee.value}`}>{fee.label}</span>
}
bottomLeftChild={
<FiatValue
disableHiddenPlaceholder
- amount={fee.networkAmount || ''}
+ amount={fee.networkAmount ?? ''}
symbol={symbol}
/>
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {feeOptions && | |
| feeOptions.map((fee, index) => ( | |
| <FeeCard | |
| key={index} | |
| value={fee.value} | |
| setSelectedLevelOption={setSelectedLevelOption} | |
| isSelected={selectedLevelOption === fee.value} | |
| changeFeeLevel={changeFeeLevel} | |
| topLeftChild={ | |
| <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> | |
| } | |
| topRightChild="" | |
| bottomLeftChild={ | |
| <FiatValue | |
| disableHiddenPlaceholder | |
| amount={fee.networkAmount || ''} | |
| symbol={symbol} | |
| showApproximationIndicator | |
| /> | |
| } | |
| bottomRightChild={ | |
| <> | |
| {formatFeePerUnit(fee?.feePerUnit)}{' '} | |
| {getFeeUnits(networkType)} | |
| </> | |
| } | |
| /> | |
| ))} | |
| {feeOptions?.map((fee) => ( | |
| <FeeCard | |
| key={fee.value} | |
| value={fee.value} | |
| setSelectedLevelOption={setSelectedLevelOption} | |
| isSelected={selectedLevelOption === fee.value} | |
| changeFeeLevel={changeFeeLevel} | |
| topLeftChild={ | |
| <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> | |
| } | |
| topRightChild="" | |
| bottomLeftChild={ | |
| <FiatValue | |
| disableHiddenPlaceholder | |
| amount={fee.networkAmount ?? ''} | |
| symbol={symbol} | |
| showApproximationIndicator | |
| /> | |
| } | |
| bottomRightChild={ | |
| <> | |
| {formatFeePerUnit(fee?.feePerUnit)}{' '} | |
| {getFeeUnits(networkType)} | |
| </> | |
| } | |
| /> | |
| ))} |
🧰 Tools
🪛 Biome (1.9.4)
[error] 171-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => | ||
| showFee && ( | ||
| <Column padding={spacings.xxxs} width="100%"> | ||
| <FeeCard | ||
| value={feeOptions[0].value} | ||
| setSelectedLevelOption={() => {}} | ||
| isSelected={true} | ||
| changeFeeLevel={() => {}} | ||
| topLeftChild={ | ||
| <span data-testid={`fee-card/${feeOptions[0].value}`}> | ||
| {feeOptions[0].label} | ||
| </span> | ||
| } | ||
| topRightChild="" | ||
| bottomLeftChild={ | ||
| <FiatValue | ||
| disableHiddenPlaceholder | ||
| amount={feeOptions[0].networkAmount || ''} | ||
| symbol={symbol} | ||
| /> | ||
| } | ||
| bottomRightChild={ | ||
| <Text variant="tertiary"> | ||
| {feeOptions[0].feePerUnit} {getFeeUnits(networkType)} | ||
| </Text> | ||
| } | ||
| /> | ||
| </Column> | ||
| ); |
There was a problem hiding this comment.
Add validation for empty feeOptions array.
The component assumes feeOptions[0] exists, which could cause runtime errors if the array is empty.
Apply this diff to add proper validation:
-const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) =>
+const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) =>
showFee && (
+ feeOptions.length > 0 && (
<Column padding={spacings.xxxs} width="100%">
<FeeCard
value={feeOptions[0].value}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => | |
| showFee && ( | |
| <Column padding={spacings.xxxs} width="100%"> | |
| <FeeCard | |
| value={feeOptions[0].value} | |
| setSelectedLevelOption={() => {}} | |
| isSelected={true} | |
| changeFeeLevel={() => {}} | |
| topLeftChild={ | |
| <span data-testid={`fee-card/${feeOptions[0].value}`}> | |
| {feeOptions[0].label} | |
| </span> | |
| } | |
| topRightChild="" | |
| bottomLeftChild={ | |
| <FiatValue | |
| disableHiddenPlaceholder | |
| amount={feeOptions[0].networkAmount || ''} | |
| symbol={symbol} | |
| /> | |
| } | |
| bottomRightChild={ | |
| <Text variant="tertiary"> | |
| {feeOptions[0].feePerUnit} {getFeeUnits(networkType)} | |
| </Text> | |
| } | |
| /> | |
| </Column> | |
| ); | |
| const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => | |
| showFee && ( | |
| feeOptions.length > 0 && ( | |
| <Column padding={spacings.xxxs} width="100%"> | |
| <FeeCard | |
| value={feeOptions[0].value} | |
| setSelectedLevelOption={() => {}} | |
| isSelected={true} | |
| changeFeeLevel={() => {}} | |
| topLeftChild={ | |
| <span data-testid={`fee-card/${feeOptions[0].value}`}> | |
| {feeOptions[0].label} | |
| </span> | |
| } | |
| topRightChild="" | |
| bottomLeftChild={ | |
| <FiatValue | |
| disableHiddenPlaceholder | |
| amount={feeOptions[0].networkAmount || ''} | |
| symbol={symbol} | |
| /> | |
| } | |
| bottomRightChild={ | |
| <Text variant="tertiary"> | |
| {feeOptions[0].feePerUnit} {getFeeUnits(networkType)} | |
| </Text> | |
| } | |
| /> | |
| </Column> | |
| ) | |
| ); |
enjojoy
force-pushed
the
fees-redesign
branch
from
77866e6 to
1203380
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/suite/src/components/wallet/Fees/FeeDetails.tsx (3)
111-139: 🛠️ Refactor suggestionImprove robustness of BitcoinDetails component.
Several improvements needed for better reliability:
- {feeOptions && - feeOptions.map((fee, index) => ( + {feeOptions?.map((fee) => ( <FeeCard - key={index} + key={fee.value} value={fee.value} setSelectedLevelOption={setSelectedLevelOption} isSelected={selectedLevelOption === fee.value} changeFeeLevel={changeFeeLevel} topLeftChild={ <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> } topRightChild={ - <>~{formatDuration(feeInfo.blockTime * (fee?.blocks || 0) * 60)}</> + <>~{formatDuration(feeInfo.blockTime * (fee?.blocks ?? 0) * 60)}</> } bottomLeftChild={ <FiatValue disableHiddenPlaceholder - amount={fee?.networkAmount || ''} + amount={fee?.networkAmount ?? ''} symbol={symbol} /> }🧰 Tools
🪛 Biome (1.9.4)
[error] 111-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
171-198: 🛠️ Refactor suggestionApply same robustness improvements to EthereumDetails.
Similar improvements needed as in BitcoinDetails.
- {feeOptions && - feeOptions.map((fee, index) => ( + {feeOptions?.map((fee) => ( <FeeCard - key={index} + key={fee.value} value={fee.value} setSelectedLevelOption={setSelectedLevelOption} isSelected={selectedLevelOption === fee.value} changeFeeLevel={changeFeeLevel} bottomLeftChild={ <FiatValue disableHiddenPlaceholder - amount={fee.networkAmount || ''} + amount={fee.networkAmount ?? ''} symbol={symbol} /> }🧰 Tools
🪛 Biome (1.9.4)
[error] 171-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
206-234: 🛠️ Refactor suggestionAdd validation for empty feeOptions array.
The component assumes feeOptions[0] exists, which could cause runtime errors.
-const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => +const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => showFee && ( + feeOptions?.length > 0 && ( <Column padding={spacings.xxxs} width="100%"> <FeeCard value={feeOptions[0].value}
🧹 Nitpick comments (2)
packages/suite/src/components/wallet/Fees/Fees.tsx (2)
1-16: Improve import organization for better maintainability.Consider grouping imports into these categories:
- React and third-party libraries
- Common types and utilities
- Components
- Local imports
import { useState } from 'react'; +import { AnimatePresence, motion } from 'framer-motion'; +import styled from 'styled-components'; import { Control, FieldErrors, UseFormGetValues, UseFormRegister, UseFormReturn, UseFormSetValue, } from 'react-hook-form'; -import { AnimatePresence, motion } from 'framer-motion'; -import styled from 'styled-components'; import { TranslationKey } from '@suite-common/intl-types'; import { NetworkSymbol, NetworkType } from '@suite-common/wallet-config';
187-187: Consider extracting network-specific logic to a separate component.The
supportsCustomFeecheck and conditional rendering of the SelectBar could be moved to a separate component to improve maintainability.+const FeeSelector = ({ networkType, isCustomFee, setIsCustomFee, changeFeeLevel }: FeeSelectorProps) => { + if (networkType === 'solana') return null; + + return ( + <SelectBarWrapper> + <SelectBar + orientation="horizontal" + selectedOption={isCustomFee ? 'custom' : 'normal'} + options={[ + { label: 'Standard', value: 'normal' }, + { label: 'Advanced', value: 'custom' }, + ]} + onChange={() => { + changeFeeLevel(isCustomFee ? 'normal' : 'custom'); + setIsCustomFee(!isCustomFee); + }} + isFullWidth + /> + </SelectBarWrapper> + ); +};Also applies to: 220-236
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/suite/src/components/wallet/Fees/CustomFee.tsx(4 hunks)packages/suite/src/components/wallet/Fees/FeeDetails.tsx(1 hunks)packages/suite/src/components/wallet/Fees/Fees.tsx(7 hunks)packages/suite/src/hooks/wallet/__fixtures__/useSendForm.ts(3 hunks)packages/suite/src/support/messages.ts(3 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInputs.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInputs.tsx
- packages/suite/src/hooks/wallet/fixtures/useSendForm.ts
- packages/suite/src/support/messages.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/components/wallet/Fees/FeeDetails.tsx
[error] 111-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 171-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (9)
packages/suite/src/components/wallet/Fees/Fees.tsx (3)
56-62: Well-structured type definition.The FeeOption type is well-defined with clear, optional properties that accommodate different network types.
239-268: Prevent layout shifts during fee option transitions.The current animation implementation might cause content jumping during transitions. Consider using a fixed height container or implementing height measurement before animation to ensure smooth transitions.
This issue was previously reported. To verify the fix:
#!/bin/bash # Search for height animation patterns that might cause layout shifts rg "height.*auto.*transition" --type ts
296-324: Inconsistent fee display across networks.The fee display logic varies between networks, particularly for Ethereum where gas price and gas limit details are not shown. This was previously flagged and should be addressed for consistency.
To verify the current fee display implementation across different networks:
#!/bin/bash # Search for fee display patterns across network types ast-grep --pattern 'networkAmount && ( $$$ <FormattedCryptoAmount $$$/> $$$ )'packages/suite/src/components/wallet/Fees/CustomFee.tsx (3)
160-160: LGTM!The increased gap and added bottom margin improve the visual spacing and readability of the form.
174-189: LGTM!Well-structured implementation of the current fee display with appropriate network-specific icons and text alignment.
192-208: LGTM!Removal of unused changeFeeLimit property simplifies the code while maintaining functionality through the useForm control.
packages/suite/src/components/wallet/Fees/FeeDetails.tsx (3)
61-93: LGTM!Well-structured FeeCard component with proper prop types and clean implementation.
156-161: LGTM!Well-implemented formatFeePerUnit helper with proper null handling and decimal formatting.
239-248: LGTM!Clean implementation of network-specific details component selection using switch statement.
| const buildFeeOptions = ( | ||
| levels: FeeLevel[], | ||
| networkType: NetworkType, | ||
| symbol: NetworkSymbol, | ||
| translationString: TranslationFunction, | ||
| composedLevels?: PrecomposedLevels | PrecomposedLevelsCardano, | ||
| ) => { | ||
| const filteredLevels = levels.filter(level => level.label !== 'custom'); | ||
|
|
||
| if (networkType === 'ethereum') { | ||
| //legacy fee format | ||
| return filteredLevels.map(level => { | ||
| const transactionInfo = composedLevels?.[level.label]; | ||
| const hasTransactionInfo = | ||
| transactionInfo !== undefined && transactionInfo.type !== 'error'; | ||
| const networkAmount = hasTransactionInfo | ||
| ? formatNetworkAmount(transactionInfo.fee, symbol) | ||
| : null; | ||
|
|
||
| return { | ||
| label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), | ||
| value: level.label, | ||
| feePerUnit: level.feePerUnit, | ||
| networkAmount, | ||
| }; | ||
| }); | ||
| } | ||
|
|
||
| if (networkType === 'bitcoin') { | ||
| return filteredLevels.map(level => { | ||
| const transactionInfo = composedLevels?.[level.label]; | ||
| const hasTransactionInfo = | ||
| transactionInfo !== undefined && transactionInfo.type !== 'error'; | ||
| const networkAmount = hasTransactionInfo | ||
| ? formatNetworkAmount(transactionInfo.fee, symbol) | ||
| : null; | ||
|
|
||
| return { | ||
| label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), | ||
| value: level.label, | ||
| blocks: level.blocks, | ||
| feePerUnit: level.feePerUnit, | ||
| networkAmount, | ||
| }; | ||
| }); | ||
| } | ||
|
|
||
| return filteredLevels.map(level => { | ||
| const transactionInfo = composedLevels?.[level.label]; | ||
| const hasTransactionInfo = | ||
| transactionInfo !== undefined && transactionInfo.type !== 'error'; | ||
| const networkAmount = hasTransactionInfo | ||
| ? formatNetworkAmount(transactionInfo.fee, symbol) | ||
| : null; | ||
|
|
||
| return { | ||
| label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), | ||
| value: level.label, | ||
| feePerUnit: level.feePerUnit, | ||
| networkAmount, | ||
| }; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication and improve error handling in buildFeeOptions.
The networkAmount calculation is duplicated across all three code paths. Additionally, there's no error handling for invalid network types.
const buildFeeOptions = (
levels: FeeLevel[],
networkType: NetworkType,
symbol: NetworkSymbol,
translationString: TranslationFunction,
composedLevels?: PrecomposedLevels | PrecomposedLevelsCardano,
) => {
const filteredLevels = levels.filter(level => level.label !== 'custom');
+
+ const getNetworkAmount = (level: FeeLevel['label']) => {
+ const transactionInfo = composedLevels?.[level];
+ return transactionInfo !== undefined && transactionInfo.type !== 'error'
+ ? formatNetworkAmount(transactionInfo.fee, symbol)
+ : null;
+ };
if (networkType === 'ethereum') {
return filteredLevels.map(level => ({
label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]),
value: level.label,
feePerUnit: level.feePerUnit,
- networkAmount: hasTransactionInfo
- ? formatNetworkAmount(transactionInfo.fee, symbol)
- : null,
+ networkAmount: getNetworkAmount(level.label),
}));
}
if (networkType === 'bitcoin') {
return filteredLevels.map(level => ({
label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]),
value: level.label,
blocks: level.blocks,
feePerUnit: level.feePerUnit,
- networkAmount: hasTransactionInfo
- ? formatNetworkAmount(transactionInfo.fee, symbol)
- : null,
+ networkAmount: getNetworkAmount(level.label),
}));
}
+ if (!Object.values(NetworkType).includes(networkType)) {
+ console.warn(`Unsupported network type: ${networkType}`);
+ }
return filteredLevels.map(level => ({
label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]),
value: level.label,
feePerUnit: level.feePerUnit,
- networkAmount: hasTransactionInfo
- ? formatNetworkAmount(transactionInfo.fee, symbol)
- : null,
+ networkAmount: getNetworkAmount(level.label),
}));
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const buildFeeOptions = ( | |
| levels: FeeLevel[], | |
| networkType: NetworkType, | |
| symbol: NetworkSymbol, | |
| translationString: TranslationFunction, | |
| composedLevels?: PrecomposedLevels | PrecomposedLevelsCardano, | |
| ) => { | |
| const filteredLevels = levels.filter(level => level.label !== 'custom'); | |
| if (networkType === 'ethereum') { | |
| //legacy fee format | |
| return filteredLevels.map(level => { | |
| const transactionInfo = composedLevels?.[level.label]; | |
| const hasTransactionInfo = | |
| transactionInfo !== undefined && transactionInfo.type !== 'error'; | |
| const networkAmount = hasTransactionInfo | |
| ? formatNetworkAmount(transactionInfo.fee, symbol) | |
| : null; | |
| return { | |
| label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), | |
| value: level.label, | |
| feePerUnit: level.feePerUnit, | |
| networkAmount, | |
| }; | |
| }); | |
| } | |
| if (networkType === 'bitcoin') { | |
| return filteredLevels.map(level => { | |
| const transactionInfo = composedLevels?.[level.label]; | |
| const hasTransactionInfo = | |
| transactionInfo !== undefined && transactionInfo.type !== 'error'; | |
| const networkAmount = hasTransactionInfo | |
| ? formatNetworkAmount(transactionInfo.fee, symbol) | |
| : null; | |
| return { | |
| label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), | |
| value: level.label, | |
| blocks: level.blocks, | |
| feePerUnit: level.feePerUnit, | |
| networkAmount, | |
| }; | |
| }); | |
| } | |
| return filteredLevels.map(level => { | |
| const transactionInfo = composedLevels?.[level.label]; | |
| const hasTransactionInfo = | |
| transactionInfo !== undefined && transactionInfo.type !== 'error'; | |
| const networkAmount = hasTransactionInfo | |
| ? formatNetworkAmount(transactionInfo.fee, symbol) | |
| : null; | |
| return { | |
| label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), | |
| value: level.label, | |
| feePerUnit: level.feePerUnit, | |
| networkAmount, | |
| }; | |
| }); | |
| }; | |
| const buildFeeOptions = ( | |
| levels: FeeLevel[], | |
| networkType: NetworkType, | |
| symbol: NetworkSymbol, | |
| translationString: TranslationFunction, | |
| composedLevels?: PrecomposedLevels | PrecomposedLevelsCardano, | |
| ) => { | |
| const filteredLevels = levels.filter(level => level.label !== 'custom'); | |
| const getNetworkAmount = (level: FeeLevel['label']) => { | |
| const transactionInfo = composedLevels?.[level]; | |
| return transactionInfo !== undefined && transactionInfo.type !== 'error' | |
| ? formatNetworkAmount(transactionInfo.fee, symbol) | |
| : null; | |
| }; | |
| if (networkType === 'ethereum') { | |
| return filteredLevels.map(level => ({ | |
| label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), | |
| value: level.label, | |
| feePerUnit: level.feePerUnit, | |
| networkAmount: getNetworkAmount(level.label), | |
| })); | |
| } | |
| if (networkType === 'bitcoin') { | |
| return filteredLevels.map(level => ({ | |
| label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), | |
| value: level.label, | |
| blocks: level.blocks, | |
| feePerUnit: level.feePerUnit, | |
| networkAmount: getNetworkAmount(level.label), | |
| })); | |
| } | |
| if (!Object.values(NetworkType).includes(networkType)) { | |
| console.warn(`Unsupported network type: ${networkType}`); | |
| } | |
| return filteredLevels.map(level => ({ | |
| label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), | |
| value: level.label, | |
| feePerUnit: level.feePerUnit, | |
| networkAmount: getNetworkAmount(level.label), | |
| })); | |
| }; |
| composedFeePerByte: string; | ||
| } | ||
|
|
||
| const getCurrentFee = (levels: FeeLevel[]) => `${levels[levels.length > 2 ? 1 : 0].feePerUnit}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety checks to getCurrentFee function.
The function needs validation to handle edge cases safely.
-const getCurrentFee = (levels: FeeLevel[]) => `${levels[levels.length > 2 ? 1 : 0].feePerUnit}`;
+const getCurrentFee = (levels: FeeLevel[]): string => {
+ if (!levels?.length) return '0';
+ const index = levels.length > 2 ? 1 : 0;
+ return `${levels[index]?.feePerUnit ?? '0'}`;
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getCurrentFee = (levels: FeeLevel[]) => `${levels[levels.length > 2 ? 1 : 0].feePerUnit}`; | |
| const getCurrentFee = (levels: FeeLevel[]): string => { | |
| if (!levels?.length) return '0'; | |
| const index = levels.length > 2 ? 1 : 0; | |
| return `${levels[index]?.feePerUnit ?? '0'}`; | |
| }; |
enjojoy
force-pushed
the
fees-redesign
branch
from
1203380 to
1549e50
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/suite/src/components/wallet/Fees/CustomFee.tsx (1)
53-53:⚠️ Potential issueAdd safety checks to getCurrentFee function.
The function needs validation to handle edge cases safely.
Apply this diff:
-const getCurrentFee = (levels: FeeLevel[]) => `${levels[levels.length > 2 ? 1 : 0].feePerUnit}`; +const getCurrentFee = (levels: FeeLevel[]): string => { + if (!levels?.length) return '0'; + const index = levels.length > 2 ? 1 : 0; + return `${levels[index]?.feePerUnit ?? '0'}`; +};packages/suite/src/components/wallet/Fees/FeeDetails.tsx (3)
171-198:⚠️ Potential issueApply same safety improvements to EthereumDetails.
The same issues exist in the EthereumDetails component.
Apply this diff:
- {feeOptions && - feeOptions.map((fee, index) => ( + {feeOptions?.map((fee) => ( <FeeCard - key={index} + key={fee.value} value={fee.value} setSelectedLevelOption={setSelectedLevelOption} isSelected={selectedLevelOption === fee.value} changeFeeLevel={changeFeeLevel} topLeftChild={ <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> } bottomLeftChild={ <FiatValue disableHiddenPlaceholder - amount={fee.networkAmount || ''} + amount={fee.networkAmount ?? ''} symbol={symbol} /> }🧰 Tools
🪛 Biome (1.9.4)
[error] 171-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
111-139:⚠️ Potential issueAdd safety checks and use optional chaining.
The component needs better error handling and safer property access.
Apply this diff:
- {feeOptions && - feeOptions.map((fee, index) => ( + {feeOptions?.map((fee) => ( <FeeCard - key={index} + key={fee.value} value={fee.value} setSelectedLevelOption={setSelectedLevelOption} isSelected={selectedLevelOption === fee.value} changeFeeLevel={changeFeeLevel} topLeftChild={ <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> } topRightChild={ - <>~{formatDuration(feeInfo.blockTime * (fee?.blocks || 0) * 60)}</> + <>~{formatDuration(feeInfo.blockTime * (fee?.blocks ?? 0) * 60)}</> } bottomLeftChild={ <FiatValue disableHiddenPlaceholder - amount={fee?.networkAmount || ''} + amount={fee?.networkAmount ?? ''} symbol={symbol} /> }🧰 Tools
🪛 Biome (1.9.4)
[error] 111-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
206-234:⚠️ Potential issueAdd validation for empty feeOptions array.
The component assumes feeOptions[0] exists, which could cause runtime errors.
Apply this diff:
-const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => +const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => showFee && ( + feeOptions.length > 0 && ( <Column padding={spacings.xxxs} width="100%"> <FeeCard value={feeOptions[0].value}packages/suite/src/components/wallet/Fees/Fees.tsx (1)
85-147: 🛠️ Refactor suggestionReduce code duplication in buildFeeOptions.
The networkAmount calculation is duplicated across all three code paths.
Apply this diff:
const buildFeeOptions = ( levels: FeeLevel[], networkType: NetworkType, symbol: NetworkSymbol, translationString: TranslationFunction, composedLevels?: PrecomposedLevels | PrecomposedLevelsCardano, ) => { const filteredLevels = levels.filter(level => level.label !== 'custom'); + + const getNetworkAmount = (level: FeeLevel['label']) => { + const transactionInfo = composedLevels?.[level]; + return transactionInfo !== undefined && transactionInfo.type !== 'error' + ? formatNetworkAmount(transactionInfo.fee, symbol) + : null; + }; + + const getBaseOptions = (level: FeeLevel) => ({ + label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), + value: level.label, + feePerUnit: level.feePerUnit, + networkAmount: getNetworkAmount(level.label), + }); if (networkType === 'ethereum') { - return filteredLevels.map(level => ({ - label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), - value: level.label, - feePerUnit: level.feePerUnit, - networkAmount: hasTransactionInfo - ? formatNetworkAmount(transactionInfo.fee, symbol) - : null, - })); + return filteredLevels.map(level => getBaseOptions(level)); } if (networkType === 'bitcoin') { return filteredLevels.map(level => ({ + ...getBaseOptions(level), - label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), - value: level.label, blocks: level.blocks, - feePerUnit: level.feePerUnit, - networkAmount: hasTransactionInfo - ? formatNetworkAmount(transactionInfo.fee, symbol) - : null, })); } - return filteredLevels.map(level => ({ - label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), - value: level.label, - feePerUnit: level.feePerUnit, - networkAmount: hasTransactionInfo - ? formatNetworkAmount(transactionInfo.fee, symbol) - : null, - })); + return filteredLevels.map(level => getBaseOptions(level)); };
🧹 Nitpick comments (2)
packages/suite/src/components/wallet/Fees/CustomFee.tsx (1)
174-189: Improve accessibility and semantic HTML structure.The current implementation nests Text components unnecessarily and could benefit from more semantic HTML structure.
Apply this diff:
- <Row justifyContent="space-between"> - <Text variant="tertiary"> - <Translation id="TR_CURRENT_FEE_CUSTOM_FEES" /> - </Text> - <Text variant="tertiary"> - <Row alignItems="center" gap={spacings.xxs}> - <Text> - {getCurrentFee(feeInfo.levels)} {getFeeUnits(networkType)} - </Text> - <Icon - name={networkType === 'ethereum' ? 'gasPump' : 'receipt'} - size="mediumLarge" - /> - </Row> - </Text> - </Row> + <Row as="section" justifyContent="space-between" aria-label="Current fee information"> + <Text as="h2" variant="tertiary"> + <Translation id="TR_CURRENT_FEE_CUSTOM_FEES" /> + </Text> + <Row as="div" alignItems="center" gap={spacings.xxs}> + <Text variant="tertiary"> + {getCurrentFee(feeInfo.levels)} {getFeeUnits(networkType)} + </Text> + <Icon + name={networkType === 'ethereum' ? 'gasPump' : 'receipt'} + size="mediumLarge" + aria-hidden="true" + /> + </Row> + </Row>packages/suite/src/components/wallet/Fees/Fees.tsx (1)
239-336: Consider extracting animation configuration.The animation configuration is duplicated between FeeDetails and CustomFee sections.
Extract the common animation configuration:
+const feeAnimationConfig = { + initial: { opacity: 0, height: 0 }, + animate: { opacity: 1, height: 'auto' }, + exit: { opacity: 0, height: 0 }, + transition: { + opacity: { duration: 0.15, ease: motionEasing.transition }, + height: { duration: 0.2, ease: motionEasing.transition }, + marginTop: { duration: 0.25, ease: motionEasing.transition }, + }, + style: { overflow: 'hidden' }, +}; <> <AnimatePresence> {!isCustomFee && ( <motion.div key="feeDetails" - initial={{ opacity: 0, height: 0 }} - animate={{ opacity: 1, height: 'auto' }} - exit={{ opacity: 0, height: 0 }} - transition={{ - opacity: { duration: 0.15, ease: motionEasing.transition }, - height: { duration: 0.2, ease: motionEasing.transition }, - marginTop: { duration: 0.25, ease: motionEasing.transition }, - }} - style={{ overflow: 'hidden' }} + {...feeAnimationConfig} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/suite/src/components/wallet/Fees/CustomFee.tsx(4 hunks)packages/suite/src/components/wallet/Fees/FeeDetails.tsx(1 hunks)packages/suite/src/components/wallet/Fees/Fees.tsx(7 hunks)packages/suite/src/hooks/wallet/__fixtures__/useSendForm.ts(3 hunks)packages/suite/src/support/messages.ts(3 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInputs.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInputs.tsx
- packages/suite/src/hooks/wallet/fixtures/useSendForm.ts
- packages/suite/src/support/messages.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/components/wallet/Fees/FeeDetails.tsx
[error] 111-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 171-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
🔇 Additional comments (1)
packages/suite/src/components/wallet/Fees/FeeDetails.tsx (1)
61-93: LGTM! Well-structured FeeCard component.The component is well-organized with clear prop types and consistent layout.
| {networkAmount && ( | ||
| <Column> | ||
| <Divider margin={{ bottom: spacings.md }} /> | ||
| <Row | ||
| gap={spacings.sm} | ||
| alignItems="baseline" | ||
| justifyContent="space-between" | ||
| > | ||
| <Text variant="tertiary"> | ||
| <Translation id="FEE" />: | ||
| </Text> | ||
| <Row gap={spacings.xxs}> | ||
| <FormattedCryptoAmount | ||
| disableHiddenPlaceholder | ||
| value={networkAmount} | ||
| symbol={symbol} | ||
| /> | ||
| <Text variant="tertiary"> | ||
| <FiatValue | ||
| disableHiddenPlaceholder | ||
| amount={networkAmount} | ||
| symbol={symbol} | ||
| showApproximationIndicator | ||
| /> | ||
| </Text> | ||
| </Row> | ||
| </Row> | ||
| </Column> | ||
| )} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fee display inconsistency: Ethereum's fee breakdown is not shown.
The component displays a singular aggregated fee (via the networkAmount) for all networks. For Ethereum transactions, consider showing the gas price and gas limit breakdown to match the detailed fee information handled in other modules.
🏁 Script executed:
#!/bin/bash
# Search for fee display patterns across the codebase
rg -A 5 'gas(Price|Limit)' --type tsLength of output: 68855
Ethereum fee breakdown not integrated in Fees component
The current implementation in packages/suite/src/components/wallet/Fees/Fees.tsx only displays an aggregated fee via networkAmount without exposing the granular Ethereum fee details (gas price and gas limit) as seen in other modules (e.g. in tx detail components and staking flows).
This inconsistency confirms the review comment’s concern.
enjojoy
force-pushed
the
fees-redesign
branch
from
1549e50 to
92b1582
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/suite/src/components/wallet/Fees/CustomFee.tsx (1)
53-53: 🛠️ Refactor suggestionAdd safety checks to getCurrentFee function.
The function needs validation to handle edge cases safely.
-const getCurrentFee = (levels: FeeLevel[]) => `${levels[levels.length > 2 ? 1 : 0].feePerUnit}`; +const getCurrentFee = (levels: FeeLevel[]): string => { + if (!levels?.length) return '0'; + const index = levels.length > 2 ? 1 : 0; + return `${levels[index]?.feePerUnit ?? '0'}`; +};packages/suite/src/components/wallet/Fees/FeeDetails.tsx (2)
206-234:⚠️ Potential issueAdd validation for empty feeOptions array.
The component assumes
feeOptions[0]exists, which could cause runtime errors if the array is empty.-const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => +const MiscDetails = ({ networkType, showFee, feeOptions, symbol }: DetailsProps) => showFee && ( + feeOptions.length > 0 && ( <Column padding={spacings.xxxs} width="100%"> <FeeCard value={feeOptions[0].value}
111-139: 🛠️ Refactor suggestionImprove array handling and React key usage.
Several improvements can enhance the component's reliability:
- Replace logical OR (
||) with nullish coalescing (??) for proper fallback handling.- Use a more stable key for the map function instead of array index.
- {feeOptions && - feeOptions.map((fee, index) => ( + {feeOptions?.map((fee) => ( <FeeCard - key={index} + key={fee.value} value={fee.value} setSelectedLevelOption={setSelectedLevelOption} isSelected={selectedLevelOption === fee.value} changeFeeLevel={changeFeeLevel} topLeftChild={ <span data-testid={`fee-card/${fee.value}`}>{fee.label}</span> } topRightChild={ - <>~{formatDuration(feeInfo.blockTime * (fee?.blocks || 0) * 60)}</> + <>~{formatDuration(feeInfo.blockTime * (fee?.blocks ?? 0) * 60)}</> } bottomLeftChild={ <FiatValue disableHiddenPlaceholder - amount={fee?.networkAmount || ''} + amount={fee?.networkAmount ?? ''} symbol={symbol} /> }🧰 Tools
🪛 Biome (1.9.4)
[error] 111-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/suite/src/components/wallet/Fees/Fees.tsx (1)
85-147: 🛠️ Refactor suggestionReduce code duplication and improve error handling in buildFeeOptions.
The networkAmount calculation is duplicated across all three code paths. Additionally, there's no error handling for invalid network types.
const buildFeeOptions = ( levels: FeeLevel[], networkType: NetworkType, symbol: NetworkSymbol, translationString: TranslationFunction, composedLevels?: PrecomposedLevels | PrecomposedLevelsCardano, ) => { const filteredLevels = levels.filter(level => level.label !== 'custom'); + + const getNetworkAmount = (level: FeeLevel['label']) => { + const transactionInfo = composedLevels?.[level]; + return transactionInfo !== undefined && transactionInfo.type !== 'error' + ? formatNetworkAmount(transactionInfo.fee, symbol) + : null; + }; if (networkType === 'ethereum') { return filteredLevels.map(level => ({ label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), value: level.label, feePerUnit: level.feePerUnit, - networkAmount: hasTransactionInfo - ? formatNetworkAmount(transactionInfo.fee, symbol) - : null, + networkAmount: getNetworkAmount(level.label), })); } if (networkType === 'bitcoin') { return filteredLevels.map(level => ({ label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), value: level.label, blocks: level.blocks, feePerUnit: level.feePerUnit, - networkAmount: hasTransactionInfo - ? formatNetworkAmount(transactionInfo.fee, symbol) - : null, + networkAmount: getNetworkAmount(level.label), })); } + if (!Object.values(NetworkType).includes(networkType)) { + console.warn(`Unsupported network type: ${networkType}`); + } return filteredLevels.map(level => ({ label: translationString(FEE_LEVELS_TRANSLATIONS[level.label]), value: level.label, feePerUnit: level.feePerUnit, - networkAmount: hasTransactionInfo - ? formatNetworkAmount(transactionInfo.fee, symbol) - : null, + networkAmount: getNetworkAmount(level.label), })); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/suite/src/components/wallet/Fees/CustomFee.tsx(4 hunks)packages/suite/src/components/wallet/Fees/FeeDetails.tsx(1 hunks)packages/suite/src/components/wallet/Fees/Fees.tsx(7 hunks)packages/suite/src/hooks/wallet/__fixtures__/useSendForm.ts(3 hunks)packages/suite/src/support/messages.ts(3 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInputs.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormInputs.tsx
- packages/suite/src/support/messages.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/components/wallet/Fees/FeeDetails.tsx
[error] 111-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 171-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (2)
packages/suite/src/components/wallet/Fees/CustomFee.tsx (1)
174-189: LGTM! UI improvements enhance fee information display.The addition of the current fee display with icon improves user experience by providing clear fee information.
packages/suite/src/hooks/wallet/__fixtures__/useSendForm.ts (1)
1727-1727: LGTM! Test fixtures updated to match new UI.The fee selection identifiers have been correctly updated to match the new fee card UI components.
Also applies to: 1760-1773
| return ( | ||
| <Column gap={spacings.xs}> | ||
| <InfoItem | ||
| direction="row" | ||
| typographyStyle="body" | ||
| label={ | ||
| networkType === 'ethereum' ? ( | ||
| <Tooltip | ||
| maxWidth={328} | ||
| hasIcon | ||
| content={<Translation id="TR_STAKE_MAX_FEE_DESC" />} | ||
| > | ||
| <Translation id={label ?? 'MAX_FEE'} /> | ||
| </Tooltip> | ||
| ) : ( | ||
| <Translation id={label ?? 'FEE'} /> | ||
| ) | ||
| } | ||
| > | ||
| {networkAmount && ( | ||
| <Row gap={spacings.md} alignItems="baseline"> | ||
| <FormattedCryptoAmount | ||
| disableHiddenPlaceholder | ||
| value={networkAmount} | ||
| symbol={symbol} | ||
| <Column gap={spacings.md}> | ||
| <Row flexWrap="wrap"> | ||
| <Row flex="1"> | ||
| <InfoItem | ||
| direction="row" | ||
| typographyStyle="body" | ||
| verticalAlignment="bottom" | ||
| label={ | ||
| <Row gap={spacings.xs}> | ||
| <Translation | ||
| id={label ?? (networkType === 'ethereum' ? 'MAX_FEE' : 'FEE')} | ||
| /> | ||
| <Tooltip | ||
| maxWidth={328} | ||
| content={ | ||
| networkType === 'ethereum' ? ( | ||
| <Translation id="TR_EVM_MAX_FEE_DESC" /> | ||
| ) : ( | ||
| <Translation id="TR_TRANSACTION_FEE_DESC" /> | ||
| ) | ||
| } | ||
| > | ||
| <Badge size="tiny"> | ||
| <Translation id="WHY_FEES" /> | ||
| </Badge> | ||
| </Tooltip> | ||
| </Row> | ||
| } | ||
| /> | ||
| </Row> | ||
| {supportsCustomFee && ( | ||
| <SelectBarWrapper> | ||
| <SelectBar | ||
| orientation="horizontal" | ||
| selectedOption={isCustomFee ? 'custom' : 'normal'} | ||
| options={[ | ||
| { label: 'Standard', value: 'normal' }, | ||
| { label: 'Advanced', value: 'custom' }, | ||
| ]} | ||
| onChange={() => { | ||
| changeFeeLevel(isCustomFee ? 'normal' : 'custom'); | ||
| setIsCustomFee(!isCustomFee); | ||
| }} | ||
| isFullWidth | ||
| /> | ||
| <Text variant="tertiary" typographyStyle="label"> | ||
| <FiatValue | ||
| disableHiddenPlaceholder | ||
| amount={networkAmount} | ||
| </SelectBarWrapper> | ||
| )} | ||
| </Row> | ||
|
|
||
| <> | ||
| <AnimatePresence> | ||
| {!isCustomFee && ( | ||
| <motion.div | ||
| key="feeDetails" | ||
| initial={{ opacity: 0, height: 0 }} | ||
| animate={{ opacity: 1, height: 'auto' }} | ||
| exit={{ opacity: 0, height: 0 }} | ||
| transition={{ | ||
| opacity: { duration: 0.15, ease: motionEasing.transition }, | ||
| height: { duration: 0.2, ease: motionEasing.transition }, | ||
| marginTop: { duration: 0.25, ease: motionEasing.transition }, | ||
| }} | ||
| style={{ overflow: 'hidden' }} | ||
| > | ||
| <FeeDetails |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider showing Ethereum fee breakdown.
The component only displays an aggregated fee via networkAmount without exposing the granular Ethereum fee details (gas price and gas limit) that are shown in other modules.
Let's verify how fee details are displayed in other modules:
Consider showing both gas price and gas limit for Ethereum transactions to maintain consistency with other parts of the application and provide users with complete fee information.
🏁 Script executed:
#!/bin/bash
# Search for fee display patterns across the codebase
rg -A 5 'gas(Price|Limit)' --type tsLength of output: 90229
Ethereum fee details are missing.
The Fees component in packages/suite/src/components/wallet/Fees/Fees.tsx only displays an aggregated fee (via networkAmount) without exposing detailed Ethereum fee parameters (such as gas price and gas limit). In contrast, other parts of the codebase (e.g., in transaction detail components like BasicTxDetails.tsx) clearly display these granular fields. This inconsistency could lead to confusion for users expecting full fee breakdown data for Ethereum transactions.
- File Affected:
packages/suite/src/components/wallet/Fees/Fees.tsx - Issue: Missing display of granular Ethereum fee details (gas price and gas limit).
| case 'ethereum': | ||
| return <EthereumDetails {...props} />; | ||
| default: | ||
| return <MiscDetails {...props} />; |
There was a problem hiding this comment.
There is missing wavy = next to fiat value 
There was a problem hiding this comment.
not in the design, but ok I'll add it
| } | ||
| bottomRightChild={ | ||
| <Text variant="tertiary"> | ||
| {feeOptions[0].feePerUnit} {getFeeUnits(networkType)} |
There was a problem hiding this comment.
For Solana, it lies. 100k lampords in send form, 5k in sign tx modal
| const gasPrice = isComposedTx | ||
| ? transactionInfo.feePerByte | ||
| : lastKnownFeePerByte || selectedLevel.feePerUnit; | ||
| return (Math.ceil(num * 100) / 100).toFixed(2); |
Description
Implement new design to fees
! The wrong colors of Badge on different elevations is a known issue and @trezor/suite-engagement is working on it
Design
Resolves #16439
Screenshots:
Bitcoin:
Bump fee:
Stake/unstake:
Solana (no custom fees):
Swap:
Buy and sell: