Refactor send modal #16465
Refactor send modal #16465
Conversation
adamhavel
requested review from
MiroslavProchazka,
komret and
jvaclavik
as code owners
adamhavel
force-pushed
the
feat/refactor-send-modal
branch
from
b897a28 to
3390737
Compare
packages/suite/src/components/suite/modals/ReduxModal/ConfirmValueModal/ConfirmValueModal.tsx
Show resolved
Hide resolved
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
...ls/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx
Outdated
Show resolved
Hide resolved
| case 'opreturn': | ||
| return <Translation id="OP_RETURN" />; | ||
| default: | ||
| return label; |
There was a problem hiding this comment.
I suggest doing an exhaustive switch instead of default fallback.
In case we add new type, the developer shall be notified by TS to handle the case here for the new type:
default: {
const _unhandledCase: never = type;
throw new Error(`Unhandled output type: ${_unhandledCase}`);
}
| }, | ||
| ]; | ||
| } else { | ||
| outputLines = [ | ||
| default: |
There was a problem hiding this comment.
and similarly here
case 'destination-tag':
return [
{
id: 'default',
type: 'default',
value,
},
];
default: {
const _unhandledCase: never = type;
throw new Error(`Unhandled output type: ${_unhandledCase}`);
}There was a problem hiding this comment.
I'm not sure this won't break something. What if there's something that relies on the default value as of now?
There was a problem hiding this comment.
If the switch is truly exhaustive it shall be safe (unless typescript lies about type)
...ls/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx
Outdated
Show resolved
Hide resolved
...eduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx
Outdated
Show resolved
Hide resolved
...eduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
First batch:
Black total box
Confirm on Trezor before scanning can be outside of QR code
Can this warning be just a note or delete it please cc @jirih-stsh
- I would not add network icon to header for now as we should resolve it for tokens somehow
- token fiat is not available
Are we fiat first company?
|
Some of this will be available in a final screen in future PR |
Token fiat wasn't available before - there's comment that it's temporarily hidden before it's fixed. I don't know the context. I'll fix the black box, thanks:) |
This is the reason: Do you know the reason? |
Screen.Recording.2025-01-24.at.13.38.35.mp4is this correct? I go back on trezor but it goes next in suite |
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Show resolved
Hide resolved
.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx
Outdated
Show resolved
Hide resolved
...xModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputElement.tsx
Show resolved
Hide resolved
|
adamhavel
force-pushed
the
feat/refactor-send-modal
branch
from
d0ab2f3 to
03f54e5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/suite-web/e2e/tests/suite/passphrase.test.ts (1)
83-86: Consider extracting the formatting logic into a test helper function.The address verification logic is correct, but since this pattern is used multiple times in the test, consider extracting it into a helper function to improve maintainability.
+const formatAddressForComparison = (address: string) => + splitStringEveryNCharacters(address, 4).join(' '); -cy.getTestElement('@modal/output-value').should( - 'contain', - splitStringEveryNCharacters(abcAddr, 4).join(' '), -); +cy.getTestElement('@modal/output-value').should( + 'contain', + formatAddressForComparison(abcAddr), +);packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (3)
41-45: Consider enhancing type safety for thedecisionprop.The
decisionprop's type could be more specific to better handle the resolution value.type TransactionReviewModalContentProps = { - decision: Deferred<boolean, string | number | undefined> | undefined; + decision: Deferred<boolean, string | number> | undefined; txInfoState: SendState | StakeState; cancelSignTx: () => void; };
115-135: Consider memoizing the analytics report function.The
reportTransactionCreatedEventfunction is recreated on each render. Consider usinguseCallbackto optimize performance.-const reportTransactionCreatedEvent = (action: 'sent' | 'copied' | 'downloaded' | 'replaced') => +const reportTransactionCreatedEvent = useCallback((action: 'sent' | 'copied' | 'downloaded' | 'replaced') => analytics.report({ type: EventType.TransactionCreated, payload: { action, symbol, tokens: outputs .filter(output => output.token?.symbol) .map(output => output.token?.symbol) .join(','), outputsCount: precomposedForm.outputs.length, broadcast: isBroadcastEnabled, bitcoinLockTime: !!options.includes('bitcoinLockTime'), ethereumData: !!options.includes('ethereumData'), rippleDestinationTag: !!options.includes('rippleDestinationTag'), ethereumNonce: !!options.includes('ethereumNonce'), selectedFee: selectedFee || 'normal', isCoinControlEnabled: precomposedForm.isCoinControlEnabled, hasCoinControlBeenOpened: precomposedForm.hasCoinControlBeenOpened, }, - }); + }), [outputs, precomposedForm, options, isBroadcastEnabled, selectedFee, symbol]);
193-222: Consider extracting bottom content into a separate component.The bottom content logic is complex with nested conditions. Extracting it would improve readability and maintainability.
+const TransactionReviewButtons = ({ + isBroadcastEnabled, + serializedTx, + isSending, + handleSend, + handleCopy, + handleDownload, + actionLabel, +}: { + isBroadcastEnabled: boolean; + serializedTx: { tx: string } | undefined; + isSending: boolean; + handleSend: () => void; + handleCopy: () => void; + handleDownload: () => void; + actionLabel: string; +}) => { + if (!isBroadcastEnabled) { + return ( + <> + <NewModal.Button + isDisabled={!serializedTx} + onClick={handleCopy} + data-testid="@send/copy-raw-transaction" + > + <Translation id="COPY_TRANSACTION_TO_CLIPBOARD" /> + </NewModal.Button> + <NewModal.Button + variant="tertiary" + isDisabled={!serializedTx} + onClick={handleDownload} + > + <Translation id="DOWNLOAD_TRANSACTION" /> + </NewModal.Button> + </> + ); + } + + return ( + <NewModal.Button + data-testid="@modal/send" + isDisabled={!serializedTx} + isLoading={isSending} + onClick={handleSend} + > + <Translation id={actionLabel} /> + </NewModal.Button> + ); +};Then use it in the modal:
bottomContent={ - !areDetailsVisible && - (isBroadcastEnabled ? ( - <NewModal.Button - data-testid="@modal/send" - isDisabled={!serializedTx} - isLoading={isSending} - onClick={handleSend} - > - <Translation id={actionLabel} /> - </NewModal.Button> - ) : ( - <> - <NewModal.Button - isDisabled={!serializedTx} - onClick={handleCopy} - data-testid="@send/copy-raw-transaction" - > - <Translation id="COPY_TRANSACTION_TO_CLIPBOARD" /> - </NewModal.Button> - <NewModal.Button - variant="tertiary" - isDisabled={!serializedTx} - onClick={handleDownload} - > - <Translation id="DOWNLOAD_TRANSACTION" /> - </NewModal.Button> - </> - )) + !areDetailsVisible && ( + <TransactionReviewButtons + isBroadcastEnabled={isBroadcastEnabled} + serializedTx={serializedTx} + isSending={isSending} + handleSend={handleSend} + handleCopy={handleCopy} + handleDownload={handleDownload} + actionLabel={actionLabel} + /> + ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
packages/components/src/components/Card/Card.tsx(6 hunks)packages/components/src/components/Card/types.tsx(1 hunks)packages/components/src/components/Card/utils.tsx(2 hunks)packages/components/src/components/DotIndicator/DotIndicator.stories.tsx(1 hunks)packages/components/src/components/DotIndicator/DotIndicator.tsx(1 hunks)packages/components/src/components/Flex/Flex.stories.tsx(1 hunks)packages/components/src/components/Flex/Flex.tsx(7 hunks)packages/components/src/index.ts(1 hunks)packages/suite-data/files/translations/en.json(1 hunks)packages/suite-desktop-core/e2e/support/common.ts(2 hunks)packages/suite-desktop-core/e2e/support/pageActions/devicePromptActions.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts(1 hunks)packages/suite-desktop-core/e2e/tests/coin-market/buy-coins.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/wallet/cardano.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/wallet/public-keys.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/wallet/send-doge.test.ts(2 hunks)packages/suite-web/e2e/tests/suite/passphrase-cardano.test.ts(2 hunks)packages/suite-web/e2e/tests/suite/passphrase-reconnection.test.ts(4 hunks)packages/suite-web/e2e/tests/suite/passphrase.test.ts(4 hunks)packages/suite/src/components/suite/Address.tsx(1 hunks)packages/suite/src/components/suite/FormattedCryptoAmount.tsx(4 hunks)packages/suite/src/components/suite/index.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/ConfirmAddressModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/ConfirmValueModal/ConfirmValueModal.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/ConfirmXpubModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewDetails.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewEvmExplanation.tsx(0 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewDetails.tsx(0 hunks)
💤 Files with no reviewable changes (2)
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewDetails.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewEvmExplanation.tsx
🚧 Files skipped from review as they are similar to previous changes (24)
- packages/components/src/index.ts
- packages/suite-desktop-core/e2e/tests/wallet/send-doge.test.ts
- packages/suite-desktop-core/e2e/support/common.ts
- packages/components/src/components/Flex/Flex.stories.tsx
- packages/suite-web/e2e/tests/suite/passphrase-cardano.test.ts
- packages/suite-desktop-core/e2e/tests/coin-market/buy-coins.test.ts
- packages/suite-desktop-core/e2e/tests/wallet/cardano.test.ts
- packages/components/src/components/Card/types.tsx
- packages/suite/src/components/suite/modals/ReduxModal/ConfirmAddressModal.tsx
- packages/components/src/components/DotIndicator/DotIndicator.tsx
- packages/suite/src/components/suite/modals/ReduxModal/ConfirmXpubModal.tsx
- packages/suite/src/components/suite/FormattedCryptoAmount.tsx
- packages/suite/src/components/suite/index.tsx
- packages/components/src/components/Card/Card.tsx
- packages/suite-data/files/translations/en.json
- packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts
- packages/suite-desktop-core/e2e/support/pageActions/devicePromptActions.ts
- packages/suite-web/e2e/tests/suite/passphrase-reconnection.test.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewDetails.tsx
- packages/components/src/components/Flex/Flex.tsx
- packages/suite-desktop-core/e2e/tests/wallet/public-keys.test.ts
- packages/components/src/components/DotIndicator/DotIndicator.stories.tsx
- packages/suite/src/components/suite/Address.tsx
- packages/suite/src/components/suite/modals/ReduxModal/ConfirmValueModal/ConfirmValueModal.tsx
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Type Checking
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Linting and formatting
- GitHub Check: Unit Tests
- 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: EAS Update
- 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: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
🔇 Additional comments (6)
packages/components/src/components/Card/utils.tsx (2)
63-63: LGTM! Good practice for consistent box sizing.Adding a default transparent border prevents layout shifts when switching between fill types.
72-81: Improve code readability and maintainability.While the implementation is functionally correct, there are a few improvements that could make it more maintainable:
- Extract theme-specific styles to improve readability
- Consider using elevation map for border colors to maintain consistency
Here's a suggested refactor:
flat: - theme.variant === 'dark' - ? css` - background: none; - border: 1px solid ${theme.borderElevation3}; - ` - : css` - background: ${theme.backgroundSurfaceElevationNegative}; - border: 1px solid ${theme.borderElevation0}; - `, + css` + ${({ theme }) => { + const styles = { + dark: { + background: 'none', + border: theme.borderElevation3, + }, + light: { + background: theme.backgroundSurfaceElevationNegative, + border: theme.borderElevation0, + }, + }; + const { background, border } = styles[theme.variant === 'dark' ? 'dark' : 'light']; + return css` + background: ${background}; + border: 1px solid ${border}; + `; + }} + `,packages/suite-web/e2e/tests/suite/passphrase.test.ts (3)
4-4: LGTM!The import of the utility function is properly placed and aligns with the refactoring goals.
114-117: LGTM! Consistent with the first verification.The address verification logic is consistently applied across all instances. This reinforces the suggestion to extract the formatting logic into a helper function.
Also applies to: 139-142
Line range hint
36-142: Verify test coverage for edge cases.The test covers the main passphrase flows well. Consider adding test cases for:
- Very long addresses that might wrap in the UI
- Addresses with special characters
- Error cases when address formatting fails
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)
Line range hint
77-80: Address TODO comment for Connect Popup special case.The TODO comment indicates missing implementation for the Connect Popup case. This should be addressed or tracked.
Would you like me to help create an issue to track this TODO or propose an implementation for the Connect Popup case?
| const handleCopy = () => { | ||
| const result = copyToClipboard(serializedTx!.tx); | ||
|
|
||
| if (typeof result !== 'string') { | ||
| dispatch( | ||
| notificationsActions.addToast({ | ||
| type: 'copy-to-clipboard', | ||
| }), | ||
| ); | ||
| } | ||
|
|
||
| reportTransactionCreatedEvent('copied'); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in the copy handler.
The copy handler doesn't handle the case where copyToClipboard fails with an error. Consider adding error notification.
const handleCopy = () => {
- const result = copyToClipboard(serializedTx!.tx);
+ try {
+ const result = copyToClipboard(serializedTx!.tx);
+ if (typeof result !== 'string') {
+ dispatch(
+ notificationsActions.addToast({
+ type: 'copy-to-clipboard',
+ }),
+ );
+ }
+ reportTransactionCreatedEvent('copied');
+ } catch (error) {
+ dispatch(
+ notificationsActions.addToast({
+ type: 'error',
+ error: 'Failed to copy transaction',
+ }),
+ );
+ }
- if (typeof result !== 'string') {
- dispatch(
- notificationsActions.addToast({
- type: 'copy-to-clipboard',
- }),
- );
- }
-
- reportTransactionCreatedEvent('copied');
};📝 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 handleCopy = () => { | |
| const result = copyToClipboard(serializedTx!.tx); | |
| if (typeof result !== 'string') { | |
| dispatch( | |
| notificationsActions.addToast({ | |
| type: 'copy-to-clipboard', | |
| }), | |
| ); | |
| } | |
| reportTransactionCreatedEvent('copied'); | |
| }; | |
| const handleCopy = () => { | |
| try { | |
| const result = copyToClipboard(serializedTx!.tx); | |
| if (typeof result !== 'string') { | |
| dispatch( | |
| notificationsActions.addToast({ | |
| type: 'copy-to-clipboard', | |
| }), | |
| ); | |
| } | |
| reportTransactionCreatedEvent('copied'); | |
| } catch (error) { | |
| dispatch( | |
| notificationsActions.addToast({ | |
| type: 'error', | |
| error: 'Failed to copy transaction', | |
| }), | |
| ); | |
| } | |
| }; |
adamhavel
force-pushed
the
feat/refactor-send-modal
branch
from
03f54e5 to
4e96727
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputElement.tsx (4)
25-36: Enhance type safety in getCardanoFingerprint function.The function could be improved to handle edge cases more explicitly and provide better type safety.
Consider this implementation:
const getCardanoFingerprint = ( tokens: Account['tokens'], symbol: string | undefined, ): string | undefined => { - if (!tokens) { + if (!tokens || !symbol) { return undefined; } - const token = tokens.find(token => token.symbol?.toLowerCase() === symbol?.toLowerCase()); + const normalizedSymbol = symbol.toLowerCase(); + const token = tokens.find(token => + token.symbol?.toLowerCase() === normalizedSymbol + ); return token?.fingerprint; };
88-88: Consider making isTabular prop configurable.The
isTabularprop is hardcoded tofalse. Consider making it configurable through component props if different formatting styles might be needed in other contexts.
95-97: Improve token address type casting.The type casting of token contract to TokenAddress could be handled more elegantly.
-tokenAddress={ - token && !isFee ? (token.contract as TokenAddress) : undefined -} +tokenAddress={ + token && !isFee && token.contract ? (token.contract as TokenAddress) : undefined +}
202-239: Extract Cardano-specific rendering logic.The Cardano-specific rendering logic makes the component harder to maintain and understand. Consider extracting it into a separate component.
Example:
const CardanoTokenDetails = ({ networkType, token, account, lineValue }: { networkType: NetworkSymbol; token?: TokenInfo; account: Account; lineValue: string; }) => { if (networkType !== 'cardano' || !token) return null; return ( <> {token.symbol && ( <Text typographyStyle="hint" as="div"> <InfoItem label={<Text variant="default"><Translation id="TR_CARDANO_FINGERPRINT_HEADLINE" /></Text>} direction="row" > <Column alignItems="flex-end" data-testid="@modal/cardano-fingerprint"> {getCardanoFingerprint(account?.tokens, token?.symbol)} </Column> </InfoItem> </Text> )} {token.decimals !== 0 && ( <Text typographyStyle="hint" as="div"> <InfoItem label={<Text variant="default"><Translation id="TR_CARDANO_TREZOR_AMOUNT_HEADLINE" /></Text>} direction="row" > <Column alignItems="flex-end" data-testid="@modal/cardano-trezor-amount"> {lineValue} </Column> </InfoItem> </Text> )} </> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/suite-desktop-core/e2e/tests/coin-market/buy-coins.test.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputElement.tsx(1 hunks)packages/suite/src/constants/suite/firmware.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/suite/src/constants/suite/firmware.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/suite-desktop-core/e2e/tests/coin-market/buy-coins.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Linting and formatting
- GitHub Check: Build libs for publishing
- GitHub Check: Unit Tests
- GitHub Check: Type Checking
- 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: build-web
- 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: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (2)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputElement.tsx (2)
113-128: LGTM! Well-structured type definitions.The type definitions are clear, well-organized, and properly typed with appropriate optional properties.
141-141: Verify the Card fillType change impact.The PR objectives mention replacing Card fill type "none" with "flat". Ensure this change is consistent across all modals and doesn't affect the visual hierarchy.
✅ Verification successful
The Card fillType change is consistent with the codebase patterns
The conditional fillType change (state === 'done' ? 'flat' : 'default') aligns with the modal design patterns, where "flat" is consistently used for content sections while maintaining visual hierarchy through conditional states.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other Card components with fillType props rg --type typescript --type typescriptreact 'fillType=("|'"'"'|{)' -B 2 -A 2Length of output: 183
Script:
#!/bin/bash # Search for Card components with fillType prop rg -t typescript -t typescriptreact '<Card.*fillType' -B 1 -A 1 # Search specifically in modal-related files rg -t typescript -t typescriptreact '<Card.*fillType' packages/suite/src/components/suite/modals -B 1 -A 1Length of output: 244
Script:
#!/bin/bash # Search for Card components with fillType in tsx files rg '<Card.*fillType' --glob '*.tsx' -B 2 -A 2 # Search specifically in modal components rg '<Card.*fillType' packages/suite/src/components/suite/modals --glob '*.tsx' -B 2 -A 2 # Search for all fillType occurrences rg 'fillType=' --glob '*.tsx' -B 2 -A 2Length of output: 16390
adamhavel
force-pushed
the
feat/refactor-send-modal
branch
from
4e96727 to
50eb248
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/suite/src/components/suite/Address.tsx (1)
25-27:⚠️ Potential issueAdd error handling for invalid address formats.
The regex pattern assumes a minimum length of 16 characters (8 for beginning + 8 for end), which could fail with shorter addresses.
- const [full, beginning, middle, end] = (value.match(/^(.{8})(.*)(.{8})$/) || []).map(part => + const [full, beginning, middle, end] = ( + (value.length >= 16 ? value.match(/^(.{8})(.*)(.{8})$/) : [value, value, '', '']) || [] + ).map(part => isChunked ? addSpacing(part) : part, );
🧹 Nitpick comments (27)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx (4)
14-23: Improve clarity and maintainability of thegetEstimatedTimefunction.Consider the following improvements:
- Extract the magic number
60into a named constant for clarity (seconds per minute).- Make the early return condition more explicit by separating network type check and matchedFeeLevel check.
+const SECONDS_PER_MINUTE = 60; + const getEstimatedTime = ( networkType: NetworkType, symbolFees: FeeInfo, tx: GeneralPrecomposedTransactionFinal, ): number | undefined => { const matchedFeeLevel = symbolFees.levels.find(item => item.feePerUnit === tx.feePerByte); - if (networkType !== 'bitcoin' || !matchedFeeLevel) return; + if (networkType !== 'bitcoin') return undefined; + if (!matchedFeeLevel) return undefined; - return matchedFeeLevel.blocks * symbolFees.blockTime * 60; + return matchedFeeLevel.blocks * symbolFees.blockTime * SECONDS_PER_MINUTE; };
76-82: Consider using explicit network type check for better readability.As discussed in the past review comments, consider replacing the negative Solana check with an explicit check for Ethereum networks, as the fee limit is only applicable to EVM chains.
- {!!tx.feeLimit && network.networkType !== 'solana' && ( + {!!tx.feeLimit && network.networkType === 'ethereum' && (
111-119: Track the IconButton margin issue.The TODO comment indicates a known issue with IconButton margins. This should be tracked and addressed.
Would you like me to create an issue to track this UI component bug? The issue would cover:
- Current behavior: IconButton not respecting margin props
- Expected behavior: IconButton should handle margin props like other components
- Temporary workaround: Using Box wrapper
84-94: Consider simplifying the fee display logic.The conditional rendering for fee display could be simplified by extracting the common parts and only conditionally rendering the different elements.
- {networkType === 'ethereum' ? ( - <Note iconName="gasPump"> - <Translation id="TR_GAS_PRICE" /> - {': '} - {fee} {getFeeUnits(network.networkType)} - </Note> - ) : ( - <Note iconName="receipt"> - {fee} {getFeeUnits(network.networkType)} - </Note> - )} + <Note iconName={networkType === 'ethereum' ? 'gasPump' : 'receipt'}> + {networkType === 'ethereum' && ( + <> + <Translation id="TR_GAS_PRICE" /> + {': '} + </> + )} + {fee} {getFeeUnits(network.networkType)} + </Note>packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputElement.tsx (3)
44-53: Consider adding exhaustive type checking in the default case.While the current implementation works, adding exhaustive type checking would make future maintenance safer by catching unhandled states at compile time.
- default: - return <DotIndicator isActive={true} />; + case 'default': + return <DotIndicator isActive={true} />; + default: { + const _unhandledCase: never = state; + throw new Error(`Unhandled state: ${_unhandledCase}`); + }
202-239: Consider extracting Cardano-specific rendering logic into a separate component.The current implementation mixes Cardano-specific token rendering with general output rendering. Extracting this into a separate component would:
- Improve code organization
- Make the main component more focused
- Make it easier to maintain network-specific features
type CardanoTokenInfoProps = { token?: TokenInfo; account: Account; value: string; }; const CardanoTokenInfo = ({ token, account, value }: CardanoTokenInfoProps) => { if (account.networkType !== 'cardano' || !token?.symbol) { return null; } return ( <> <Text typographyStyle="hint" as="div"> <InfoItem label={ <Text variant="default"> <Translation id="TR_CARDANO_FINGERPRINT_HEADLINE" /> </Text> } direction="row" > <Column alignItems="flex-end" data-testid="@modal/cardano-fingerprint"> {getCardanoFingerprint(account?.tokens, token?.symbol)} </Column> </InfoItem> </Text> {token.decimals !== 0 && ( <Text typographyStyle="hint" as="div"> <InfoItem label={ <Text variant="default"> <Translation id="TR_CARDANO_TREZOR_AMOUNT_HEADLINE" /> </Text> } direction="row" > <Column alignItems="flex-end" data-testid="@modal/cardano-trezor-amount"> {value} </Column> </InfoItem> </Text> )} </> ); };
141-141: Consider simplifying the Card's fillType prop.The ternary expression can be made more readable by extracting the logic.
- <Card paddingType="none" fillType={state === 'done' ? 'flat' : 'default'}> + const cardFillType = state === 'done' ? 'flat' : 'default'; + <Card paddingType="none" fillType={cardFillType}>packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewTotalOutput.tsx (2)
Line range hint
33-43: Consider extracting fee label mapping to a constant.The network-to-fee-label mapping could be extracted into a constant object to improve maintainability and reduce the complexity of the function.
+const NETWORK_FEE_LABELS: Record<NetworkType, string> = { + ethereum: 'MAX_FEE', + solana: 'TR_TX_FEE', + bitcoin: 'TR_INCLUDING_FEE', + ripple: 'TR_INCLUDING_FEE', + cardano: 'TR_INCLUDING_FEE', +}; + const feeLabel = ((network: NetworkType) => { - switch (network) { - case 'ethereum': - return 'MAX_FEE'; - case 'solana': - return 'TR_TX_FEE'; - default: - return 'TR_INCLUDING_FEE'; - } + return NETWORK_FEE_LABELS[network]; })(networkType);
104-130: Consider performance optimizations.The component could benefit from memoization:
- Memoize the lines computation to prevent unnecessary recalculations
- Memoize the device selector if it's used in multiple components
+import { useMemo } from 'react'; export const TransactionReviewTotalOutput = ({ account, state, precomposedTx, stakeType, isRbf, }: TransactionReviewTotalOutputProps) => { const device = useSelector(selectSelectedDevice); if (!device) { return null; } const { networkType, symbol } = account; - const lines = getLines(device, networkType, precomposedTx, isRbf, stakeType); + const lines = useMemo( + () => getLines(device, networkType, precomposedTx, isRbf, stakeType), + [device, networkType, precomposedTx, isRbf, stakeType] + );packages/suite/src/components/suite/Address.tsx (4)
14-14: Consider memoizing the helper function.The
addSpacingfunction could benefit from memoization if it's called frequently with the same inputs.-const addSpacing = (value: string) => value.match(/.{1,4}/g)?.join(' ') || value; +const addSpacing = React.useCallback( + (value: string) => value.match(/.{1,4}/g)?.join(' ') || value, + [] +);
16-19: Consider adding validation props.The component could benefit from additional props for address validation and error handling.
export type AddressProps = { value: string; isTruncated?: boolean; + isValid?: boolean; + onValidationError?: (error: string) => void; };
31-39: Optimize copy handler with useCallback.The
handleCopyfunction should be memoized to prevent unnecessary re-renders.- const handleCopy = (e: React.ClipboardEvent) => { + const handleCopy = React.useCallback((e: React.ClipboardEvent) => { const selection = window.getSelection()?.toString(); e.preventDefault(); e.clipboardData?.setData( 'text/plain', selection?.replace(placeholder, middle).replace(/\s/g, '') ?? value, ); - }; + }, [middle, placeholder, value]);
41-42: Add error boundary or fallback UI.Consider adding error handling for cases where the address formatting fails.
- return <AddressWrapper onCopy={handleCopy}>{formattedValue ?? value}</AddressWrapper>; + return ( + <AddressWrapper onCopy={handleCopy}> + {try { + formattedValue ?? value + } catch (error) { + console.error('Address formatting failed:', error); + return value; + }} + </AddressWrapper> + );packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx (3)
Line range hint
20-29: Consider using an exhaustive switch with a type assertion.The switch statement could be improved by adding a type assertion to ensure all network types are handled.
const getFeeLabel = (networkType: NetworkType) => { switch (networkType) { case 'ethereum': return 'MAX_FEE'; case 'solana': return 'EXPECTED_FEE'; default: { - return 'FEE'; + const _exhaustiveCheck: never = networkType; + return 'FEE'; // This line is still reachable for known network types } } };
49-101: Consider extracting translation IDs to constants.The function is well-implemented with exhaustive type checking, but the translation IDs could be extracted to named constants for better maintainability.
+const TRANSLATION_IDS = { + LOCKTIME_TIMESTAMP: 'LOCKTIME_TIMESTAMP', + LOCKTIME_BLOCKHEIGHT: 'LOCKTIME_BLOCKHEIGHT', + SUMMARY: 'TR_SUMMARY', + TOKEN: 'TR_TOKEN', + CONTRACT: 'TR_CONTRACT', + // ... other IDs +} as const; const getOutputTitle = ( type: ReviewOutput['type'], networkType: NetworkType, value: string, isRbf: boolean, stakeType: StakeType | undefined, ): ReactNode | undefined => { // ... rest of the implementation using TRANSLATION_IDS };
216-267: Consider performance optimizations and explicit handling.A few suggestions to improve the component:
- Memoize complex calculations
- Make the early return condition more explicit
+import { useMemo } from 'react'; export const TransactionReviewOutput = ({ type, state, label, value, value2, token, account, stakeType, isRbf, }: TransactionReviewOutputProps) => { const { networkType, symbol } = account; const { translationString } = useTranslation(); - const isFiatVisible = + const isFiatVisible = useMemo(() => ['fee', 'amount', 'gas', 'fee-replace', 'reduce-output'].includes(type) && - !isTestnet(symbol); + !isTestnet(symbol), + [type, symbol] + ); - const outputTitle = getOutputTitle(type, networkType, value, isRbf, stakeType); + const outputTitle = useMemo( + () => getOutputTitle(type, networkType, value, isRbf, stakeType), + [type, networkType, value, isRbf, stakeType] + ); // prevents double label when bumping stake type txs if (type === 'address' && isRbf && stakeType) { - return null; + return null; // Skipping render for stake type RBF transactions } // ... rest of the implementation };packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewDetails.tsx (3)
21-21: Consider using more specific types instead ofany.The
prettifyfunction uses theRecord<any, any>type which is not type-safe. Consider using more specific types orRecord<string, unknown>as a safer alternative.-const prettify = (json: Record<any, any>) => JSON.stringify(json, null, 2); +const prettify = (json: Record<string, unknown>) => JSON.stringify(json, null, 2);
24-24: Add a TODO comment with more context.The comment "BTC-only, TODO: eth/ripple" could be more descriptive about what needs to be implemented for ETH and Ripple.
- if (tx.inputs.length === 0) return null; // BTC-only, TODO: eth/ripple + if (tx.inputs.length === 0) return null; // BTC-only + // TODO: Implement transaction details view for ETH and Ripple transactions which don't have inputs
35-40: Consider extracting repeated Card pattern into a reusable component.The same Card pattern with padding and Text styling is repeated three times. Consider extracting it into a reusable component to reduce code duplication.
const CodeCard = ({ children }: { children: React.ReactNode }) => ( <Card paddingType="small"> <Text typographyStyle="label" as="div"> <Pre>{children}</Pre> </Text> </Card> );Then use it like:
- <Card paddingType="small"> - <Text typographyStyle="label" as="div"> - <Pre>{prettify(tx.inputs)}</Pre> - </Text> - </Card> + <CodeCard>{prettify(tx.inputs)}</CodeCard>Also applies to: 42-47, 50-55
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx (2)
101-101: Avoid ref assignment in expressions.The ref assignment in the expression can be confusing and is flagged by static analysis. Consider using a separate function for ref handling.
- <Wrapper key={index} ref={ref => (outputRefs.current[index] = ref)}> + <Wrapper + key={index} + ref={ref => { + outputRefs.current[index] = ref; + }} + >🧰 Tools
🪛 Biome (1.9.4)
[error] 101-101: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
83-89: Consider debouncing scroll behavior.The scroll behavior in the useEffect hook might cause multiple unnecessary scrolls when buttonRequestsCount changes rapidly. Consider debouncing the scroll operation.
import { debounce } from 'lodash'; // Add this before the component: const debouncedScroll = debounce((element: HTMLElement | null, behavior: ScrollBehavior) => { element?.scrollIntoView({ behavior }); }, 100); // Then in useEffect: useEffect(() => { if (buttonRequestsCount - 1 === outputs.length || signedTx) { debouncedScroll(totalOutputRef.current, signedTx ? 'instant' : 'smooth'); } else if (buttonRequestsCount !== 0) { debouncedScroll(outputRefs.current[buttonRequestsCount - 1], 'smooth'); } return () => debouncedScroll.cancel(); }, [buttonRequestsCount, outputs.length, signedTx]);packages/components/src/components/Flex/Flex.tsx (2)
176-177: Consider using nullish coalescing for gap fallbacks.Using the nullish coalescing operator would be more explicit about the fallback behavior for undefined values.
- rowGap = gap, - columnGap = gap, + rowGap ?? = gap, + columnGap ?? = gap,
134-134: Consider handling zero gaps more efficiently.When both gaps are zero, we could avoid setting the gap property altogether to reduce CSS output.
- gap: ${({ $rowGap, $columnGap }) => `${$rowGap}px ${$columnGap}px`}; + gap: ${({ $rowGap, $columnGap }) => + $rowGap === 0 && $columnGap === 0 ? 'unset' : `${$rowGap}px ${$columnGap}px`};packages/components/src/components/Card/Card.tsx (1)
69-72: Consider reducing transition duration.A 0.5s duration for hover transitions might feel sluggish. Material Design guidelines suggest using shorter durations (0.2s-0.3s) for hover effects to maintain a snappy feel.
transition: - background 0.5s, - border 0.5s, - box-shadow 0.5s; + background 0.2s, + border 0.2s, + box-shadow 0.2s;packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (3)
115-135: Consider extracting analytics reporting to a separate utility.The analytics reporting logic could be moved to a separate utility function to improve maintainability and reusability.
+// transactionAnalytics.ts +export const reportTransactionCreated = ({ + action, + outputs, + precomposedForm, + symbol, + isBroadcastEnabled, + options, + selectedFee, +}: TransactionAnalyticsParams) => { + analytics.report({ + type: EventType.TransactionCreated, + payload: { + action, + symbol, + tokens: outputs + .filter(output => output.token?.symbol) + .map(output => output.token?.symbol) + .join(','), + outputsCount: precomposedForm.outputs.length, + broadcast: isBroadcastEnabled, + bitcoinLockTime: !!options.includes('bitcoinLockTime'), + ethereumData: !!options.includes('ethereumData'), + rippleDestinationTag: !!options.includes('rippleDestinationTag'), + ethereumNonce: !!options.includes('ethereumNonce'), + selectedFee: selectedFee || 'normal', + isCoinControlEnabled: precomposedForm.isCoinControlEnabled, + hasCoinControlBeenOpened: precomposedForm.hasCoinControlBeenOpened, + }, + }); +};
180-192: Consider extracting description logic to a separate component.The description prop contains complex conditional rendering logic that could be extracted to improve readability.
+const TransactionReviewModalDescription = ({ + areDetailsVisible, + tx, + account, + broadcast, + onDetailsClick, + stakeType, +}: DescriptionProps) => + !areDetailsVisible && ( + <TransactionReviewSummary + tx={tx} + account={account} + broadcast={broadcast} + onDetailsClick={onDetailsClick} + stakeType={stakeType} + /> + );
193-221: Consider extracting button logic to a separate component.The bottom content contains complex conditional rendering for buttons that could be extracted to improve maintainability.
+const TransactionReviewModalButtons = ({ + isBroadcastEnabled, + serializedTx, + isSending, + onSend, + onCopy, + onDownload, + actionLabel, +}: ButtonsProps) => + !areDetailsVisible && + (isBroadcastEnabled ? ( + <NewModal.Button + data-testid="@modal/send" + isDisabled={!serializedTx} + isLoading={isSending} + onClick={onSend} + > + <Translation id={actionLabel} /> + </NewModal.Button> + ) : ( + <> + <NewModal.Button + isDisabled={!serializedTx} + onClick={onCopy} + data-testid="@send/copy-raw-transaction" + > + <Translation id="COPY_TRANSACTION_TO_CLIPBOARD" /> + </NewModal.Button> + <NewModal.Button + variant="tertiary" + isDisabled={!serializedTx} + onClick={onDownload} + > + <Translation id="DOWNLOAD_TRANSACTION" /> + </NewModal.Button> + </> + ));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
packages/components/src/components/Card/Card.tsx(6 hunks)packages/components/src/components/Card/types.tsx(1 hunks)packages/components/src/components/Card/utils.tsx(2 hunks)packages/components/src/components/DotIndicator/DotIndicator.stories.tsx(1 hunks)packages/components/src/components/DotIndicator/DotIndicator.tsx(1 hunks)packages/components/src/components/Flex/Flex.stories.tsx(1 hunks)packages/components/src/components/Flex/Flex.tsx(7 hunks)packages/components/src/index.ts(1 hunks)packages/suite-data/files/translations/en.json(1 hunks)packages/suite-desktop-core/e2e/support/common.ts(2 hunks)packages/suite-desktop-core/e2e/support/pageActions/devicePromptActions.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts(1 hunks)packages/suite-desktop-core/e2e/tests/coin-market/buy-coins.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/wallet/cardano.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/wallet/public-keys.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/wallet/send-doge.test.ts(2 hunks)packages/suite-web/e2e/tests/suite/passphrase-cardano.test.ts(2 hunks)packages/suite-web/e2e/tests/suite/passphrase-reconnection.test.ts(4 hunks)packages/suite-web/e2e/tests/suite/passphrase.test.ts(4 hunks)packages/suite/src/components/suite/Address.tsx(1 hunks)packages/suite/src/components/suite/FormattedCryptoAmount.tsx(4 hunks)packages/suite/src/components/suite/index.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/ConfirmAddressModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/ConfirmValueModal/ConfirmValueModal.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/ConfirmXpubModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewDetails.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewEvmExplanation.tsx(0 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewDetails.tsx(0 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputElement.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewStepIndicator.tsx(0 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewTotalOutput.tsx(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/ChangeFee.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/Detail/AdvancedTxDetails/AdvancedTxDetails.tsx(1 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/wallet/send/TotalSent/TotalSent.tsx(1 hunks)packages/suite/src/views/wallet/trading/common/TradingTransactions/TradingTransaction/TradingTransactionContainer.tsx(1 hunks)suite-common/wallet-utils/src/reviewTransactionUtils.ts(2 hunks)
💤 Files with no reviewable changes (3)
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewStepIndicator.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewDetails.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewEvmExplanation.tsx
🚧 Files skipped from review as they are similar to previous changes (26)
- packages/components/src/index.ts
- packages/suite/src/views/wallet/trading/common/TradingTransactions/TradingTransaction/TradingTransactionContainer.tsx
- packages/suite-web/e2e/tests/suite/passphrase-cardano.test.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/Detail/AdvancedTxDetails/AdvancedTxDetails.tsx
- packages/suite-desktop-core/e2e/support/common.ts
- packages/suite/src/components/suite/index.tsx
- packages/components/src/components/Flex/Flex.stories.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/ChangeFee.tsx
- packages/components/src/components/DotIndicator/DotIndicator.tsx
- packages/suite-desktop-core/e2e/tests/coin-market/buy-coins.test.ts
- packages/suite-desktop-core/e2e/tests/wallet/send-doge.test.ts
- packages/components/src/components/Card/types.tsx
- packages/suite/src/views/wallet/send/TotalSent/TotalSent.tsx
- packages/components/src/components/DotIndicator/DotIndicator.stories.tsx
- packages/suite-desktop-core/e2e/tests/wallet/public-keys.test.ts
- packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts
- packages/suite/src/components/suite/modals/ReduxModal/ConfirmXpubModal.tsx
- packages/suite-web/e2e/tests/suite/passphrase-reconnection.test.ts
- packages/suite-desktop-core/e2e/tests/wallet/cardano.test.ts
- packages/suite-desktop-core/e2e/support/pageActions/devicePromptActions.ts
- suite-common/wallet-utils/src/reviewTransactionUtils.ts
- packages/suite-data/files/translations/en.json
- packages/suite/src/components/suite/modals/ReduxModal/ConfirmValueModal/ConfirmValueModal.tsx
- packages/suite/src/components/suite/modals/ReduxModal/ConfirmAddressModal.tsx
- packages/suite/src/components/suite/FormattedCryptoAmount.tsx
- packages/suite/src/support/messages.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx
[error] 101-101: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Unit Tests
- GitHub Check: Type Checking
- GitHub Check: Other Checks
- GitHub Check: Linting and formatting
- GitHub Check: Build libs for publishing
- GitHub Check: Releases revision Checks
- 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: EAS Update
- 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: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
🔇 Additional comments (16)
packages/suite-web/e2e/tests/suite/passphrase.test.ts (2)
4-4: LGTM! Clean import of the utility function.The addition of the
splitStringEveryNCharactersutility function import helps centralize the address formatting logic.
83-86: LGTM! Consistent approach to address verification.The changes improve the test by:
- Using a consistent approach across all address verifications
- Centralizing the address formatting logic in a utility function
- Making the test more maintainable by removing duplicated formatting logic
The implementation correctly verifies the formatted addresses in the modal output.
Also applies to: 114-117, 139-142
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputElement.tsx (2)
25-36: LGTM! Well-implemented utility function.The
getCardanoFingerprintfunction correctly handles:
- Case-insensitive token symbol comparison
- Null safety checks
- Clear return type
64-110: LGTM! Well-structured Value component with proper type safety.The component:
- Handles different value types appropriately
- Includes proper exhaustive type checking
- Correctly formats token and network amounts
- Has clear separation of concerns
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewTotalOutput.tsx (3)
1-19: LGTM! Well-organized imports with clear separation of concerns.The imports are logically grouped and properly sourced from common packages.
96-102: LGTM! Clear and well-defined props interface.The props interface is concise and properly typed with all necessary properties.
48-65: Verify the handling of unknown staking claim value.The special case for unknown staking claim value only returns the fee line. Please verify if this is the intended behavior for all staking claim scenarios.
✅ Verification successful
Special handling of claim transactions is verified and appears intentional
The codebase consistently shows special handling for claim operations, including fee-only displays in certain scenarios. This pattern is particularly evident in RBF (Replace-By-Fee) claim transactions where showing only the fee line aligns with the overall architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances where staking claim values are handled rg -A 5 "stakeType.*===.*'claim'" .Length of output: 5010
packages/suite/src/components/suite/Address.tsx (1)
1-6: LGTM! Clean imports and well-defined constant.The imports are appropriate for the component's needs, and the truncation placeholder is well-defined as a constant.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx (2)
31-47: LGTM! Clear and type-safe mapping implementation.The stake type to translation key mapping is well-structured and type-safe.
102-214: Verify edge cases in amount handling.The function handles various output types well, but there are some potential edge cases to consider:
- Empty strings for
valueandvalue2in amount-related cases- Negative values in fee calculations
Run this script to check for potential edge cases:
✅ Verification successful
Edge cases in amount handling are properly covered by tests
The codebase has comprehensive test coverage for amount-related edge cases:
- Empty strings and invalid amounts are properly validated
- Negative values are prevented through validation
- Fee calculations handle edge cases like MAX_SAFE_INTEGER
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for amount/fee value handling in tests rg -A 5 "amount|fee" --glob "**/*test*" --type tsLength of output: 91594
packages/components/src/components/Card/utils.tsx (1)
72-81: Consider using elevation map for border colors.The flat fill type implementation could benefit from using the elevation map for border colors to maintain consistency with the theme system.
packages/components/src/components/Card/Card.tsx (3)
3-3: LGTM! Clean import organization.The imports are well-organized and the new imports (
useTheme,ElevationContext) align with the component's enhanced functionality.Also applies to: 22-22
41-41: LGTM! Consistent fill type handling.The background condition now checks for 'flat' instead of 'none', aligning with the PR's objective to replace the Card fill type.
149-155: LGTM! Smart elevation handling based on theme.The conditional rendering with ElevationContext correctly handles elevation differences between dark and light themes.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (2)
147-159: Improve error handling in the copy handler.The copy handler should handle potential errors from
copyToClipboard.
138-140: Document Solana-specific behavior.The special case for Solana network type needs documentation explaining why it requires different loading state handling.
| const AddressWrapper = styled.p` | ||
| text-wrap: balance; | ||
| font-variant-numeric: tabular-nums; | ||
| letter-spacing: 0; | ||
| `; |
There was a problem hiding this comment.
💡 Codebase verification
Add progressive enhancement for experimental CSS property
The text-wrap: balance property is experimental with limited browser support (Chrome 114+, Safari 16.5+). Consider using @supports for progressive enhancement:
const AddressWrapper = styled.p`
font-variant-numeric: tabular-nums;
letter-spacing: 0;
@supports (text-wrap: balance) {
text-wrap: balance;
}
`;🔗 Analysis chain
Consider browser compatibility for CSS properties.
The text-wrap: balance property is experimental and has limited browser support. Consider adding a fallback or progressive enhancement strategy.
Run this script to check browser support:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check browser support for CSS properties
curl -s "https://raw.githubusercontent.com/mdn/browser-compat-data/main/css/properties/text-wrap.json" | jq '.css.properties["text-wrap"].support'
Length of output: 151
Script:
#!/bin/bash
# Check browser support for both CSS properties
echo "Checking text-wrap support:"
curl -s "https://raw.githubusercontent.com/mdn/browser-compat-data/main/css/properties/text-wrap/balance.json" | jq '.css.properties["text-wrap-balance"].support'
echo -e "\nChecking font-variant-numeric support:"
curl -s "https://raw.githubusercontent.com/mdn/browser-compat-data/main/css/properties/font-variant-numeric.json" | jq '.css.properties["font-variant-numeric"].support'
Length of output: 558
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx (1)
102-214: Consider simplifying similar output line cases.The
getOutputLinesfunction has several cases that follow similar patterns. Consider grouping similar cases to reduce code duplication.For example, these cases could be grouped:
- case 'txid': - case 'contract': - case 'opreturn': - case 'destination-tag': - case 'locktime': - return [ - { - id: type, - type: 'data', - value, - }, - ]; + case 'txid': + case 'contract': + case 'opreturn': + case 'destination-tag': + case 'locktime': { + const commonDataOutput = { + id: type, + type: 'data', + value, + }; + return [commonDataOutput]; + }packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewTotalOutput.tsx (3)
Line range hint
33-43: Consider extracting network-specific fee label logic.The network-specific fee label logic could be moved to a separate function or constant mapping for better maintainability.
+const NETWORK_FEE_LABELS: Record<NetworkType, string> = { + ethereum: 'MAX_FEE', + solana: 'TR_TX_FEE', + bitcoin: 'TR_INCLUDING_FEE', + ripple: 'TR_INCLUDING_FEE', + cardano: 'TR_INCLUDING_FEE', +}; + const feeLabel = ((network: NetworkType) => { - switch (network) { - case 'ethereum': - return 'MAX_FEE'; - case 'solana': - return 'TR_TX_FEE'; - default: - return 'TR_INCLUDING_FEE'; - } + return NETWORK_FEE_LABELS[network]; })(networkType);
51-83: Enhance type safety for amount and fee lines.Consider creating a type union for line types and extracting common line properties to improve type safety and maintainability.
type LineType = 'amount' | 'fee' | 'total'; interface BaseOutputLine { id: string; label: React.ReactNode; value: string; type: LineType; } const createLine = ( id: string, labelId: string, value: string, type: LineType ): BaseOutputLine => ({ id, label: <Translation id={labelId} />, value, type, });
111-115: Consider handling device check at a higher level.The device null check could be moved up the component tree to avoid unnecessary component instantiation and improve reusability.
This component could be wrapped by a higher-order component or the check could be moved to the parent component:
// In parent component if (!device) { return null; } return ( <TransactionReviewTotalOutput account={account} state={state} precomposedTx={precomposedTx} stakeType={stakeType} isRbf={isRbf} device={device} // Pass device as prop instead of using selector /> );packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (2)
Line range hint
61-71: Consider memoizing the precomposedForm selector.The selector combines multiple conditions and could benefit from memoization to prevent unnecessary recalculations.
- const precomposedForm = useSelector(state => - isStakeState(txInfoState) - ? selectStakePrecomposedForm(state) - : selectPrecomposedSendForm(state), - ); + const precomposedForm = useSelector( + state => + isStakeState(txInfoState) + ? selectStakePrecomposedForm(state) + : selectPrecomposedSendForm(state), + (prev, next) => prev?.selectedFee === next?.selectedFee + );
193-222: Improve readability of conditional rendering in bottomContent.The nested conditional rendering using
&&and ternary operators could be simplified for better maintainability.- bottomContent={ - !areDetailsVisible && - (isBroadcastEnabled ? ( - <NewModal.Button - data-testid="@modal/send" - isDisabled={!serializedTx} - isLoading={isSending} - onClick={handleSend} - > - <Translation id={actionLabel} /> - </NewModal.Button> - ) : ( - <> - <NewModal.Button - isDisabled={!serializedTx} - onClick={handleCopy} - data-testid="@send/copy-raw-transaction" - > - <Translation id="COPY_TRANSACTION_TO_CLIPBOARD" /> - </NewModal.Button> - <NewModal.Button - variant="tertiary" - isDisabled={!serializedTx} - onClick={handleDownload} - > - <Translation id="DOWNLOAD_TRANSACTION" /> - </NewModal.Button> - </> - )) - } + bottomContent={ + areDetailsVisible ? null : ( + isBroadcastEnabled ? ( + <NewModal.Button + data-testid="@modal/send" + isDisabled={!serializedTx} + isLoading={isSending} + onClick={handleSend} + > + <Translation id={actionLabel} /> + </NewModal.Button> + ) : ( + <> + <NewModal.Button + isDisabled={!serializedTx} + onClick={handleCopy} + data-testid="@send/copy-raw-transaction" + > + <Translation id="COPY_TRANSACTION_TO_CLIPBOARD" /> + </NewModal.Button> + <NewModal.Button + variant="tertiary" + isDisabled={!serializedTx} + onClick={handleDownload} + > + <Translation id="DOWNLOAD_TRANSACTION" /> + </NewModal.Button> + </> + ) + ) + }packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputElement.tsx (3)
44-53: Add exhaustive type checking to the switch statement.While the component logic is correct, adding exhaustive type checking would prevent future maintenance issues if new states are added.
const Status = ({ state }: { state: TransactionReviewOutputElementProps['state'] }) => { switch (state) { case 'done': return <Icon size={spacings.md} variant="primary" name="check" />; case 'pending': return <DotIndicator />; - default: + case 'default': return <DotIndicator isActive={true} />; + default: { + const _unhandledCase: never = state; + throw new Error(`Unhandled state: ${_unhandledCase}`); + } } };
64-110: Consider extracting token amount formatting logic.The token amount formatting logic in the
Valuecomponent could be simplified by extracting it into a separate function for better maintainability.+const formatTokenAmount = ( + value: string, + token: TokenInfo | undefined, + symbol: NetworkSymbol, + isFee: boolean +) => { + const isTokenAmount = !isFee && token; + return isTokenAmount + ? formatAmount(value, token.decimals) + : formatNetworkAmount(value, symbol); +}; const Value = ({ value, type, symbol, token, isFee, isFiatVisible }: ValueProps) => { switch (type) { case 'address': return <Address value={value} />; case 'data': return <DataWrapper>{value}</DataWrapper>; case 'total': case 'fee': case 'amount': { - const isTokenAmount = !isFee && token; - const formattedValue = isTokenAmount - ? formatAmount(value, token.decimals) - : formatNetworkAmount(value, symbol); + const formattedValue = formatTokenAmount(value, token, symbol, isFee);
202-239: Consider extracting Cardano-specific rendering logic.The Cardano-specific rendering logic could be extracted into a separate component to improve maintainability and reduce the complexity of the main component.
+const CardanoTokenDetails = ({ + networkType, + token, + account, + line, +}: { + networkType: string; + token?: TokenInfo; + account: Account; + line: OutputElementLine; +}) => { + if (networkType !== 'cardano') return null; + + return ( + <> + {token?.symbol && ( + <Text typographyStyle="hint" as="div"> + <InfoItem + label={ + <Text variant="default"> + <Translation id="TR_CARDANO_FINGERPRINT_HEADLINE" /> + </Text> + } + direction="row" + > + <Column alignItems="flex-end" data-testid="@modal/cardano-fingerprint"> + {getCardanoFingerprint(account?.tokens, token?.symbol)} + </Column> + </InfoItem> + </Text> + )} + {token && token.decimals !== 0 && ( + <Text typographyStyle="hint" as="div"> + <InfoItem + label={ + <Text variant="default"> + <Translation id="TR_CARDANO_TREZOR_AMOUNT_HEADLINE" /> + </Text> + } + direction="row" + > + <Column alignItems="flex-end" data-testid="@modal/cardano-trezor-amount"> + {line.value} + </Column> + </InfoItem> + </Text> + )} + </> + ); +}; // In the main component's render: - {networkType === 'cardano' && token?.symbol && ( - // ... Cardano fingerprint rendering - )} - {networkType === 'cardano' && token && token.decimals !== 0 && ( - // ... Cardano amount rendering - )} + <CardanoTokenDetails + networkType={networkType} + token={token} + account={account} + line={line} + />packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx (2)
14-23: Document the estimation calculation logic.The
getEstimatedTimefunction's calculation logic (blocks * blockTime * 60) would benefit from a comment explaining the formula and units.Add JSDoc comment:
+/** + * Calculates estimated confirmation time in seconds for Bitcoin transactions. + * Formula: blocks * blockTime (minutes) * 60 (seconds) + * @returns undefined for non-Bitcoin networks, estimated time in seconds for Bitcoin + */ const getEstimatedTime = ( networkType: NetworkType, symbolFees: FeeInfo, tx: GeneralPrecomposedTransactionFinal, ): number | undefined => {
48-51: Consider memoizing expensive calculations.The network, fee, and estimate time calculations could benefit from memoization to prevent unnecessary recalculations on re-renders.
Consider using
useMemo:- const network = networks[account.symbol]; - const { symbol, accountType, index, networkType } = account; - const fee = getFee(networkType, tx); - const estimateTime = getEstimatedTime(networkType, fees[account.symbol], tx); + const network = useMemo(() => networks[account.symbol], [account.symbol]); + const { symbol, accountType, index, networkType } = account; + const fee = useMemo(() => getFee(networkType, tx), [networkType, tx]); + const estimateTime = useMemo( + () => getEstimatedTime(networkType, fees[account.symbol], tx), + [networkType, fees, account.symbol, tx] + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/components/src/components/DotIndicator/DotIndicator.tsx(1 hunks)packages/suite-desktop-core/e2e/tests/coin-market/buy-coins.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/wallet/send-doge.test.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/ConfirmValueModal/ConfirmValueModal.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/ConfirmXpubModal.tsx(0 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewDetails.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputElement.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewTotalOutput.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/suite/src/components/suite/modals/ReduxModal/ConfirmXpubModal.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/suite-desktop-core/e2e/tests/wallet/send-doge.test.ts
- packages/suite-desktop-core/e2e/tests/coin-market/buy-coins.test.ts
- packages/components/src/components/DotIndicator/DotIndicator.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewDetails.tsx
- packages/suite/src/components/suite/modals/ReduxModal/ConfirmValueModal/ConfirmValueModal.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx
[error] 101-101: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Unit Tests
- GitHub Check: Type Checking
- GitHub Check: Build libs for publishing
- GitHub Check: Linting and formatting
- GitHub Check: Other Checks
- GitHub Check: Releases revision Checks
- 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: prepare_android_test_app
- 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: Analyze with CodeQL (javascript)
- GitHub Check: EAS Update
🔇 Additional comments (20)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutput.tsx (4)
1-19: LGTM! Well-organized imports.The imports are logically grouped and all dependencies appear to be necessary for the component's functionality.
Line range hint
20-48: LGTM! Type-safe utility functions.Both utility functions are well-implemented with proper type safety:
getFeeLabeluses an exhaustive switch for network typesgetDisplayModeStringsMapprovides a clean mapping for stake operations
49-99: LGTM! Well-structured output title handling.The
getOutputTitlefunction effectively handles all output types with proper type safety using an exhaustive switch pattern.
216-267: LGTM! Clean component implementation.The component is well-structured with:
- Clear props interface
- Proper edge case handling for stake transactions
- Comprehensive visibility checks for fiat values
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewTotalOutput.tsx (2)
Line range hint
1-18: LGTM! Well-organized imports.The imports are logically grouped and all dependencies are properly utilized in the code.
96-102: LGTM! Clear and well-typed props interface.The props interface is well-defined with appropriate types for each property.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (2)
Line range hint
3-45: LGTM! Clean imports and type definitions.The imports are well-organized and the type definitions are concise and focused.
147-159: Improve error handling in the copy handler.The copy handler should handle potential failures gracefully.
const handleCopy = () => { - const result = copyToClipboard(serializedTx!.tx); + try { + const result = copyToClipboard(serializedTx!.tx); + if (typeof result !== 'string') { + dispatch( + notificationsActions.addToast({ + type: 'copy-to-clipboard', + }), + ); + } + reportTransactionCreatedEvent('copied'); + } catch (error) { + dispatch( + notificationsActions.addToast({ + type: 'error', + error: 'Failed to copy transaction', + }), + ); + } - if (typeof result !== 'string') { - dispatch( - notificationsActions.addToast({ - type: 'copy-to-clipboard', - }), - ); - } - - reportTransactionCreatedEvent('copied'); };packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputList.tsx (5)
5-8: LGTM! Clean imports and well-structured props interface.The imports are logically organized, and the props interface is well-typed with clear property definitions.
Also applies to: 17-26
28-42: LGTM! Clear and predictable state determination logic.The
getStatefunction uses a straightforward if-else structure, making it easy to understand and maintain. The logic handles all possible states (done, default, pending) based on the index and button request count.
48-61: Consider extracting SectionHeading into a separate component.The
SectionHeadingcomponent has its own logic and rendering. Moving it to a separate file would improve modularity and make the code more maintainable.Create a new file
SectionHeading.tsx:import { H4 } from '@trezor/components'; import { spacings } from '@trezor/theme'; import { Translation } from 'src/components/suite'; import { ReviewOutput } from '@suite-common/wallet-types'; type SectionHeadingProps = { output: ReviewOutput; index: number; }; export const SectionHeading = ({ output, index }: SectionHeadingProps) => ( <H4 margin={{ top: index === 0 ? spacings.zero : spacings.xs }}> {output.type === 'address' ? ( <Translation id="TR_SEND_RECIPIENT_ADDRESS" values={{ index: index + 1, }} /> ) : ( <Translation id="TR_SUMMARY" /> )} </H4> );
83-89: LGTM! Well-implemented scrolling behavior.The useEffect hook handles scrolling behavior appropriately, considering both signed transactions (instant scroll) and in-progress transactions (smooth scroll).
92-143: Consider breaking down the component for better maintainability.The component has grown quite large and handles multiple responsibilities:
- Output list rendering
- Scroll management
- State management
- Conditional rendering
- Banner display
Consider splitting it into smaller, focused components:
- OutputList (handling the map operation)
- TotalOutputSection
- SolanaWarningBanner
Would you like me to help generate the component structure for this refactoring?
🧰 Tools
🪛 Biome (1.9.4)
[error] 101-101: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputElement.tsx (4)
25-36: LGTM! Well-structured utility function.The
getCardanoFingerprintfunction is type-safe and handles the token lookup efficiently.
38-41: LGTM! Good typography choices.The
DataWrapperstyled component uses appropriate CSS properties for numerical data display:
word-break: break-allprevents overflowfont-variant-numeric: tabular-numsensures consistent number widthletter-spacing: 0maintains readability
113-128: LGTM! Well-structured type definitions.The types are well-defined with clear interfaces for both the output line elements and component props.
141-141: Good use of conditional styling.The Card's
fillTypeprop is appropriately set based on the state, providing visual feedback to users.packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx (3)
26-32: LGTM! Clean props structure.The props type definition is well-structured with clear types and necessary properties.
76-82: Improve network type check readability.As discussed in previous reviews, the network type check could be more explicit.
Consider using a direct check for Ethereum-based networks:
- {!!tx.feeLimit && network.networkType !== 'solana' && ( + {!!tx.feeLimit && network.networkType === 'ethereum' && (
110-119: Address TODO comment about IconButton margin.The TODO comment indicates an issue with IconButton margins that should be resolved.
Let's verify if this is a known issue:
| .indexOf(output); | ||
|
|
||
| return ( | ||
| <Wrapper key={index} ref={ref => (outputRefs.current[index] = ref)}> |
There was a problem hiding this comment.
Fix ref assignment in expression.
The current ref assignment in an expression can be confusing and triggers a static analysis warning.
Refactor the ref assignment to be more explicit:
-<Wrapper key={index} ref={ref => (outputRefs.current[index] = ref)}>
+<Wrapper
+ key={index}
+ ref={ref => {
+ outputRefs.current[index] = ref;
+ }}
+>📝 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.
| <Wrapper key={index} ref={ref => (outputRefs.current[index] = ref)}> | |
| <Wrapper | |
| key={index} | |
| ref={ref => { | |
| outputRefs.current[index] = ref; | |
| }} | |
| > |
🧰 Tools
🪛 Biome (1.9.4)
[error] 101-101: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
|
QA NOK i dont see amount in send modal 🤯
Info:
|
Screenshots:
Basic send flow
Ethereum
Token
Staking
RBF
Receive
Summary by CodeRabbit
Release Notes
New Features
Addresscomponent for displaying addresses with optional truncation.DotIndicatorcomponent for visual state representation.TransactionReviewDetailscomponent for displaying transaction details.Improvements
Bug Fixes
Chores