Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add error state for RBF when previous transaction gets mined befor user sends the modal #16878

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

peter-sanderson
Copy link
Contributor

@peter-sanderson peter-sanderson commented Feb 6, 2025

Partially addresses #16875

This PR handles the situation in RBF flow, when block is mined when modal is opened.

Case #1

  • User opens modal and submits the RBF
  • Then we await confirmation on device for amount, address, fee, ...
    image
  • Then, before user confirms the block is mined and original Tx gets confirmed
  • User sees this error screen:
    image

Case #2

  • User opens modal and submits the RBF
  • user does all the confirmation on the device and is about to broadcast TX
    image
  • Then, before user confirms the block is mined and original Tx gets confirmed
  • User sees this error screen:
    image

Copy link

coderabbitai bot commented Feb 6, 2025

Walkthrough

This pull request introduces a specialized mechanism for handling errors in replace-by-fee transactions. It adds a new thunk (replaceByFeeErrorThunk) that utilizes a dedicated error constant (RBF_ERROR_ALREADY_MINED) to detect when a transaction has already been mined. The error-handling logic in the transaction send workflow is enhanced so that, upon detecting this error, the user interface retains an open modal for further user review rather than dismissing it immediately. Additionally, modal components and related type definitions are updated to support this new error state, and a new middleware is implemented to intercept relevant actions and trigger the error handling cascade. Updates to message definitions ensure that the error feedback is clear and reflect the precise transaction state. Type and action modifications further integrate this functionality into the overall transaction management flow.

Possibly related issues

Possibly related PRs

Suggested labels

bug, +Invity

Suggested reviewers

  • tomasklim
  • MiroslavProchazka
  • adderpositive

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

github-actions bot commented Feb 6, 2025

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 26
  • More info

Learn more about 𝝠 Expo Github Action

@peter-sanderson peter-sanderson force-pushed the cancel-transaction-screen-2-error branch 3 times, most recently from 6d7a742 to cae81c4 Compare February 7, 2025 12:31
@peter-sanderson peter-sanderson marked this pull request as ready for review February 7, 2025 12:31
@@ -227,7 +227,13 @@ export const signAndPushSendFormTransactionThunk = createThunk(
);

if (isRejected(signResponse)) {
// close modal manually since UI.CLOSE_UI.WINDOW was blocked
// Do not close the modal, as we need that modal to display the error state.
if (signResponse.payload?.message === RBF_ERROR_ALREADY_MINED) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly hack, that unfortunately have to employ. The similar mechanism is used in wallet-switcher.

This is currently the only way, how to send some date from place where cancel on device happen to the place where result of cancelled operation is awaited

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
packages/suite/src/actions/wallet/send/replaceByFeeErrorThunk.ts (1)

9-20: Consider adding error logging.

While the error handling flow is correct, consider adding error logging to help with debugging and monitoring.

 export const replaceByFeeErrorThunk = createThunk(
     `${MODULE_PREFIX}/replaceByFeeErrorThunk`,
     (_, { dispatch }) => {
+        console.error('RBF transaction failed: Original transaction already mined');
         TrezorConnect.cancel(RBF_ERROR_ALREADY_MINED);

         dispatch(
             openModal({
                 type: 'review-transaction-rbf-previous-transaction-mined-error',
             }),
         );
     },
 );
packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts (1)

10-33: Consider adding early returns for better readability.

The middleware implementation is correct but could be more readable with early returns.

 export const replaceByFeeErrorMiddleware =
     (api: MiddlewareAPI<Dispatch, AppState>) =>
     (next: Dispatch) =>
     (action: Action): Action => {
         next(action);
 
-        if (transactionsActions.addTransaction.match(action)) {
-            const { transactions } = action.payload;
+        if (!transactionsActions.addTransaction.match(action)) {
+            return action;
+        }
 
-            const precomposedTx = api.getState().wallet.send?.precomposedTx;
+        const { transactions } = action.payload;
+        const precomposedTx = api.getState().wallet.send?.precomposedTx;
 
-            if (precomposedTx !== undefined && isRbfTransaction(precomposedTx)) {
-                const addedTransaction = transactions.find(
-                    tx => tx.txid === precomposedTx.prevTxid,
-                );
+        if (!precomposedTx || !isRbfTransaction(precomposedTx)) {
+            return action;
+        }
 
-                if (addedTransaction !== undefined && addedTransaction.blockHeight !== undefined) {
-                    api.dispatch(replaceByFeeErrorThunk());
-                }
-            }
+        const addedTransaction = transactions.find(
+            tx => tx.txid === precomposedTx.prevTxid,
+        );
+
+        if (addedTransaction?.blockHeight !== undefined) {
+            api.dispatch(replaceByFeeErrorThunk());
         }
 
         return action;
     };
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)

19-19: Consider adding prop types validation.

While the implementation is correct, consider adding prop-types validation for better runtime type checking.

+import PropTypes from 'prop-types';

 export const TransactionReviewModal = ({ type, decision }: TransactionReviewModalProps) => {
     // ... existing code ...
 };

+TransactionReviewModal.propTypes = {
+    type: PropTypes.oneOf(['review-transaction', 'sign-transaction', 'review-transaction-rbf-previous-transaction-mined-error']).isRequired,
+    decision: PropTypes.shape({
+        resolve: PropTypes.func,
+        reject: PropTypes.func,
+    }),
+};

Also applies to: 38-38

packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)

176-216: Consider extracting button variants to constants.

While the BottomContent component is well-structured, the button variants ('tertiary') could be extracted to named constants for better maintainability.

+const BUTTON_VARIANTS = {
+  TERTIARY: 'tertiary',
+} as const;

 const BottomContent = () => {
   if (isRbfConfirmedError) {
     return (
-      <NewModal.Button variant="tertiary" onClick={onCancel}>
+      <NewModal.Button variant={BUTTON_VARIANTS.TERTIARY} onClick={onCancel}>
         <Translation id="TR_CLOSE" />
       </NewModal.Button>
     );
   }
   // ... rest of the code
 };
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eab0cc7 and cae81c4.

📒 Files selected for processing (15)
  • packages/suite/src/actions/wallet/send/replaceByFeeErrorThunk.ts (1 hunks)
  • packages/suite/src/actions/wallet/send/sendFormThunks.ts (2 hunks)
  • packages/suite/src/actions/wallet/send/sendThunksConsts.ts (1 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (2 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (6 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx (2 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionFailed.tsx (0 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx (2 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ReplaceByFeeFailedOriginalTxConfirmed.tsx (1 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx (1 hunks)
  • packages/suite/src/middlewares/wallet/index.ts (2 hunks)
  • packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts (1 hunks)
  • packages/suite/src/support/messages.ts (1 hunks)
  • suite-common/suite-types/src/modal.ts (1 hunks)
  • suite-common/wallet-core/src/transactions/transactionsActions.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionFailed.tsx
✅ Files skipped from review due to trivial changes (1)
  • packages/suite/src/actions/wallet/send/sendThunksConsts.ts
🧰 Additional context used
📓 Learnings (1)
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx (1)
Learnt from: Lemonexe
PR: trezor/trezor-suite#16525
File: packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx:60-76
Timestamp: 2025-02-03T17:19:25.374Z
Learning: The CancelTransactionModal component should handle error states from composeCancelTransactionThunk by showing CancelTransactionFailed component and disable the cancel button during loading.
⏰ Context from checks skipped due to timeout of 90000ms (11)
  • GitHub Check: build-web
  • GitHub Check: prepare_android_test_app
  • 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: build-web
  • GitHub Check: Analyze with CodeQL (javascript)
  • GitHub Check: Setup and Cache Dependencies
  • GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
🔇 Additional comments (19)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx (2)

30-30: LGTM! Making onDetailsClick optional improves component flexibility.

The change to make onDetailsClick optional is a good practice as it allows the component to handle states where the details interaction isn't needed.


110-120: LGTM! Proper guard against undefined handler.

The condition is well-structured with:

  • Efficient check ordering (cheap check first)
  • Proper guard against undefined handler
  • Consistent with the optional prop type change
packages/suite/src/actions/wallet/send/replaceByFeeErrorThunk.ts (1)

7-7: LGTM! Clear and descriptive error constant.

The error constant follows naming conventions and clearly describes the error scenario.

packages/suite/src/middlewares/wallet/index.ts (1)

18-18: LGTM! Middleware properly integrated.

The replaceByFeeErrorMiddleware is correctly imported and added to the middleware chain.

Also applies to: 32-32

packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (1)

11-17: LGTM! Clear type definition.

The type definition is well-structured and properly includes the new RBF error case.

suite-common/wallet-core/src/transactions/transactionsActions.ts (2)

24-38: Well-structured type definitions!

The new type definitions improve type safety and code maintainability by clearly defining the structure of the action's parameters and return value.


47-47: Clean type implementation!

Good use of the newly defined types in the action creator's signature.

packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ReplaceByFeeFailedOriginalTxConfirmed.tsx (2)

12-30: Well-organized type and mapping definitions!

Good use of TypeScript's Record type for mapping different scenarios to their respective titles, descriptions, and help links. The type parameter effectively handles both RBF and cancel transaction cases.


32-51: Clean and consistent UI implementation!

The component follows good UI/UX practices:

  • Clear visual hierarchy with warning icon
  • Consistent spacing using theme values
  • Helpful links to documentation
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx (1)

24-24: Good integration of the new error component!

The replacement of CancelTransactionFailed with ReplaceByFeeFailedOriginalTxConfirmed aligns with the learning about error handling in the modal. The implementation correctly handles the confirmed transaction state.

Also applies to: 102-104

suite-common/suite-types/src/modal.ts (1)

60-63: Well-defined user context type!

The new type 'review-transaction-rbf-previous-transaction-mined-error' with optional decision property aligns perfectly with the PR's objective of handling RBF scenarios when a block is mined while the modal is open.

packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx (1)

109-110: LGTM! Clean addition of RBF error handling.

The new case for handling RBF errors when previous transactions are mined follows the existing pattern and maintains consistency by reusing the TransactionReviewModal component.

packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (4)

48-49: LGTM! Well-documented prop addition.

The comment about potential future extension into a generic error type shows good foresight for maintainability.


106-115: LGTM! Clear error handling logic.

The onCancel function cleanly handles both RBF error and normal cases, maintaining proper state cleanup.


218-240: LGTM! Clean component extraction.

The Content component effectively encapsulates rendering logic and improves code organization.


244-254: LGTM! Appropriate conditional rendering.

The ConfirmOnDevice component is correctly hidden during RBF error state, improving user experience.

packages/suite/src/actions/wallet/send/sendFormThunks.ts (2)

36-37: LGTM! Clear imports for error handling.

The imports for RBF error constant and module prefix improve code organization.


230-237: LGTM! Well-documented error handling.

The error handling logic clearly distinguishes between RBF mining errors and other cases, with helpful comments explaining the flow.

packages/suite/src/support/messages.ts (1)

6445-6462: LGTM! New error messages for transaction cancellation/replacement flows.

The new message definitions are well-structured and follow the existing conventions. The messages clearly explain to users why their transaction cancellation or replacement failed (because the transaction was already confirmed on the network).

@peter-sanderson peter-sanderson force-pushed the cancel-transaction-screen-2-error branch from cae81c4 to c9e26c9 Compare February 7, 2025 12:35
Copy link
Contributor

@Lemonexe Lemonexe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay. The redux logic is quite convoluted, but as discussed personally, I don't have an idea how to do it more systematically.

Please see the comments before merging, one condition needs to be changed I think.

Tested and works well!

import { Translation, TranslationKey } from '../../../../Translation';
import { TrezorLink } from '../../../../TrezorLink';

type ReplaceByFeeFailedOriginalTxConfirmedParams = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny nit: Props, not Params

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -42,12 +44,16 @@ type TransactionReviewModalContentProps = {
decision: Deferred<boolean, string | number | undefined> | undefined;
txInfoState: SendState | StakeState;
cancelSignTx: () => void;

// Just bool for now, but could be extended into generic error in the future
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment is needed - it's trivial

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

);
}

if (!areDetailsVisible && isBroadcastEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I look at the original condition it says
!areDetailsVisible && (isBroadcastEnabled ? <JSX1 /> : <JSX2 />)
You did not keep the same logic, is this intentional?
I believe this upholds original behavior:

Suggested change
if (!areDetailsVisible && isBroadcastEnabled) {
if (areDetailsVisible) return null;
if (isBroadcastEnabled) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you're right, nice catch, thank you, fixed

@peter-sanderson peter-sanderson force-pushed the cancel-transaction-screen-2-error branch from c9e26c9 to 9f1c31a Compare February 7, 2025 15:46
@peter-sanderson peter-sanderson force-pushed the cancel-transaction-screen-2-error branch from 9f1c31a to c87c86c Compare February 7, 2025 15:53
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (3)

104-113: Consider restructuring the onCancel function for better readability.

While the logic is correct, the function could be more readable by handling the normal case first.

Consider this refactor:

 const onCancel = () => {
-    if (isRbfConfirmedError) {
-        dispatch(modalActions.onCancel());
-    }
-
     if (isActionAbortable || serializedTx) {
         cancelSignTx();
         decision?.resolve(false);
     }
+
+    if (isRbfConfirmedError) {
+        dispatch(modalActions.onCancel());
+    }
 };

174-218: Consider moving BottomContent outside the main component.

While the component logic is correct, it's currently recreated on every render of the parent component. Moving it outside would improve performance.

Consider this approach:

const BottomContent: React.FC<{
    isRbfConfirmedError?: boolean;
    areDetailsVisible: boolean;
    isBroadcastEnabled: boolean;
    serializedTx?: { tx: string };
    isSending: boolean;
    onCancel: () => void;
    handleSend: () => void;
    handleCopy: () => void;
    handleDownload: () => void;
    actionLabel: string;
}> = ({ /* props */ }) => {
    // existing implementation
};

220-242: Consider moving Content outside the main component.

Similar to BottomContent, this component would benefit from being moved outside the parent component to prevent unnecessary recreations.

Consider this approach:

const Content: React.FC<{
    areDetailsVisible: boolean;
    isRbfConfirmedError?: boolean;
    precomposedTx: PrecomposedTransaction;
    serializedTx?: { tx: string };
    // ... other required props
}> = ({ /* props */ }) => {
    // existing implementation
};
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9f1c31a and c87c86c.

📒 Files selected for processing (14)
  • packages/suite/src/actions/wallet/send/replaceByFeeErrorThunk.ts (1 hunks)
  • packages/suite/src/actions/wallet/send/sendFormThunks.ts (2 hunks)
  • packages/suite/src/actions/wallet/send/sendThunksConsts.ts (1 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (2 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (6 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionFailed.tsx (0 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx (2 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ReplaceByFeeFailedOriginalTxConfirmed.tsx (1 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx (1 hunks)
  • packages/suite/src/middlewares/wallet/index.ts (2 hunks)
  • packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts (1 hunks)
  • packages/suite/src/support/messages.ts (1 hunks)
  • suite-common/suite-types/src/modal.ts (1 hunks)
  • suite-common/wallet-core/src/transactions/transactionsActions.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionFailed.tsx
🚧 Files skipped from review as they are similar to previous changes (12)
  • packages/suite/src/actions/wallet/send/sendThunksConsts.ts
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
  • suite-common/suite-types/src/modal.ts
  • packages/suite/src/middlewares/wallet/index.ts
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx
  • packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ReplaceByFeeFailedOriginalTxConfirmed.tsx
  • packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx
  • suite-common/wallet-core/src/transactions/transactionsActions.ts
  • packages/suite/src/actions/wallet/send/replaceByFeeErrorThunk.ts
  • packages/suite/src/actions/wallet/send/sendFormThunks.ts
  • packages/suite/src/support/messages.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
  • 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: build-web
  • GitHub Check: Setup and Cache Dependencies
  • GitHub Check: build-web
  • GitHub Check: Analyze with CodeQL (javascript)
  • GitHub Check: prepare_android_test_app
  • GitHub Check: EAS Update
🔇 Additional comments (2)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (2)

36-36: LGTM! Props and imports are well-structured.

The new import and prop addition are correctly implemented to support the RBF error state handling.

Also applies to: 47-47


244-279: LGTM! Clean and well-structured conditional rendering.

The changes effectively handle the RBF error state while maintaining good separation of concerns. The conditional rendering is clear and follows React best practices.

@peter-sanderson peter-sanderson force-pushed the cancel-transaction-screen-2-error branch from c87c86c to ab34593 Compare February 7, 2025 16:01
@peter-sanderson peter-sanderson force-pushed the cancel-transaction-screen-2-error branch from ab34593 to 5933cb7 Compare February 10, 2025 08:49
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (1)

104-113: Consider simplifying the onCancel function.

The function can be made more concise by handling both cases together.

-    const onCancel = () => {
-        if (isRbfConfirmedError) {
-            dispatch(modalActions.onCancel());
-        }
-
-        if (isActionAbortable || serializedTx) {
-            cancelSignTx();
-            decision?.resolve(false);
-        }
-    };
+    const onCancel = () => {
+        if (isRbfConfirmedError) {
+            dispatch(modalActions.onCancel());
+            return;
+        }
+        
+        if (isActionAbortable || serializedTx) {
+            cancelSignTx();
+            decision?.resolve(false);
+        }
+    };
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ab34593 and 5933cb7.

📒 Files selected for processing (14)
  • packages/suite/src/actions/wallet/send/replaceByFeeErrorThunk.ts (1 hunks)
  • packages/suite/src/actions/wallet/send/sendFormThunks.ts (2 hunks)
  • packages/suite/src/actions/wallet/send/sendThunksConsts.ts (1 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx (2 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (6 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionFailed.tsx (0 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx (2 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ReplaceByFeeFailedOriginalTxConfirmed.tsx (1 hunks)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx (1 hunks)
  • packages/suite/src/middlewares/wallet/index.ts (2 hunks)
  • packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts (1 hunks)
  • packages/suite/src/support/messages.ts (1 hunks)
  • suite-common/suite-types/src/modal.ts (1 hunks)
  • suite-common/wallet-core/src/transactions/transactionsActions.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionFailed.tsx
🚧 Files skipped from review as they are similar to previous changes (12)
  • packages/suite/src/actions/wallet/send/sendThunksConsts.ts
  • packages/suite/src/middlewares/wallet/index.ts
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
  • packages/suite/src/middlewares/wallet/replaceByFeeErrorMiddleware.ts
  • suite-common/suite-types/src/modal.ts
  • packages/suite/src/actions/wallet/send/replaceByFeeErrorThunk.ts
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ReplaceByFeeFailedOriginalTxConfirmed.tsx
  • packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModal.tsx
  • packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransactionModal.tsx
  • suite-common/wallet-core/src/transactions/transactionsActions.ts
  • packages/suite/src/actions/wallet/send/sendFormThunks.ts
  • packages/suite/src/support/messages.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
  • 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: prepare_android_test_app
  • GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
  • GitHub Check: build-web
  • GitHub Check: build-web
  • GitHub Check: Setup and Cache Dependencies
  • GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (4)
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx (4)

25-25: LGTM! Props and imports are well-organized.

The new imports and prop isRbfConfirmedError are correctly added to support the RBF error state handling.

Also applies to: 36-36, 47-47


174-218: LGTM! Well-structured component extraction.

The BottomContent component cleanly handles different modal states and improves code organization.


220-242: LGTM! Clean component extraction.

The Content component effectively manages different view states and maintains good separation of concerns.


244-279: LGTM! Clean and maintainable render logic.

The main render logic effectively uses the extracted components and correctly handles the RBF error state.

@peter-sanderson peter-sanderson merged commit 2db04c3 into develop Feb 10, 2025
33 checks passed
@peter-sanderson peter-sanderson deleted the cancel-transaction-screen-2-error branch February 10, 2025 09:41
@peter-sanderson peter-sanderson self-assigned this Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🤝 Needs QA
Development

Successfully merging this pull request may close these issues.

2 participants