Skip to content

Commit faee9e7

Browse files
vinistevamvinistevam
authored
fix: correct gas modal stacking on cancel/speed-up (#28232)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** On iOS, opening **Edit network fee** from the cancel/speed-up flow could show nothing because `GasFeeModal` (its own `react-native-modal`) was rendered as a sibling **outside** the cancel/speed-up `Modal`, so the gas UI could present behind the existing modal window. This change renders `GasFeeModal` **inside** the same `react-native-modal` as the cancel/speed-up sheet so stacking is correct. Gesture handling is aligned with that structure: outer modal no longer uses swipe-to-dismiss props that fought the inner `BottomSheet`; the sheet receives `onClose` for swipe-to-dismiss, and the header close path only runs `onCloseBottomSheet()` (avoiding double `onClose`). The redundant `ScrollView` wrapper was removed for this fixed-height content. Also ports the **transaction modifiability** behavior from the speed-up/cancel UX work: `useCancelSpeedupGas` exposes `isTransactionModifiable` when status is `unapproved` or `submitted`. The cancel/speed-up modal skips gas seeding when the tx is not modifiable, hides the edit affordance, and closes the nested gas modal if the tx becomes non-modifiable while open. <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: Fixed network fee edit icon not responding to taps on iOS in the cancel and speed-up transaction modal. ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/CONF-1123 ## **Manual testing steps** ```gherkin Feature: Cancel and speed-up modal network fee edit on iOS Scenario: User opens gas editor from network fee row Given the user is on iOS with a pending EVM transaction And the user opens the cancel or speed-up transaction modal And the modal shows the network fee row with an edit control When the user taps the network fee edit (pencil) control Then the gas fee editor modal is shown ``` ## **Screenshots/Recordings** https://github.com/user-attachments/assets/b4356a9b-1137-4c97-ac7b-9693ddc45a3b https://github.com/user-attachments/assets/d1053dbd-c6b9-4ac4-8ed7-c80dc20b0853 https://github.com/user-attachments/assets/8e000371-b694-47c2-8850-d1e618f4cb9a <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes modal nesting/close behavior for the cancel/speed-up flow and adds status-based gating for gas edits, which could affect user ability to edit/confirm transactions if edge cases are missed. Covered by added unit tests, but UI/transaction-state interactions are moderately risky. > > **Overview** > Fixes iOS cancel/speed-up gas editor visibility by **nesting `GasFeeModal` inside the same `react-native-modal`** as the cancel/speed-up bottom sheet, and removing conflicting swipe-to-dismiss props so the sheet owns dismissal. > > Adds `isTransactionModifiable` to `useCancelSpeedupGas` (true only for `unapproved`/`submitted`) and uses it to **skip gas seeding**, **hide the “edit gas” affordance**, and **auto-close the gas modal** if the transaction becomes non-modifiable. Updates/extends unit tests for both the modal behavior and the new status flag. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit af35d7a. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent a8d18ab commit faee9e7

5 files changed

Lines changed: 153 additions & 51 deletions

File tree

‎app/components/Views/confirmations/components/modals/cancel-speedup-modal/cancel-speedup-modal.test.tsx‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ const defaultGasValues = {
6666
networkFeeFiat: '$1.80',
6767
nativeTokenSymbol: 'ETH',
6868
isInitialGasReady: true,
69+
isTransactionModifiable: true,
6970
};
7071

7172
jest.mock('../../../hooks/gas/useCancelSpeedupGas', () => ({
@@ -269,6 +270,45 @@ describe('CancelSpeedupModal', () => {
269270
});
270271
});
271272

273+
it('does not show edit gas when transaction is not modifiable', () => {
274+
mockedUseCancelSpeedupGas.mockReturnValue({
275+
...defaultGasValues,
276+
isTransactionModifiable: false,
277+
});
278+
279+
const { queryByTestId } = renderWithProvider(
280+
<CancelSpeedupModal {...defaultProps} />,
281+
{ state: baseState },
282+
);
283+
284+
expect(queryByTestId('cancel-speedup-edit-gas')).toBeNull();
285+
});
286+
287+
it('dismisses gas modal when transaction becomes non-modifiable', async () => {
288+
mockedUseCancelSpeedupGas.mockReturnValue({
289+
...defaultGasValues,
290+
isTransactionModifiable: true,
291+
});
292+
293+
const { getByTestId, queryByTestId, rerender } = renderWithProvider(
294+
<CancelSpeedupModal {...defaultProps} />,
295+
{ state: baseState },
296+
);
297+
298+
fireEvent.press(getByTestId('cancel-speedup-edit-gas'));
299+
expect(getByTestId('gas-fee-modal')).toBeOnTheScreen();
300+
301+
mockedUseCancelSpeedupGas.mockReturnValue({
302+
...defaultGasValues,
303+
isTransactionModifiable: false,
304+
});
305+
rerender(<CancelSpeedupModal {...defaultProps} />);
306+
307+
await waitFor(() => {
308+
expect(queryByTestId('gas-fee-modal')).toBeNull();
309+
});
310+
});
311+
272312
it('does not call onConfirm when isInitialGasReady is false', () => {
273313
mockedUseCancelSpeedupGas.mockReturnValue({
274314
...defaultGasValues,

‎app/components/Views/confirmations/components/modals/cancel-speedup-modal/cancel-speedup-modal.tsx‎

Lines changed: 58 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import React, { useCallback, useEffect, useRef, useState } from 'react';
2-
import { Pressable, ScrollView, StyleSheet } from 'react-native';
2+
import { Pressable, StyleSheet } from 'react-native';
33
import Modal from 'react-native-modal';
44
import {
55
isEIP1559Transaction,
@@ -180,12 +180,13 @@ export function CancelSpeedupModal({
180180
networkFeeFiat,
181181
nativeTokenSymbol,
182182
isInitialGasReady,
183+
isTransactionModifiable,
183184
} = useCancelSpeedupGas({ txId: tx?.id });
184185

185186
// Seed the transaction with bump params when cancel/speed up modal opens so the gas modal shows suggested values.
186187
// Stores the original gas as previousGas first (prevents re-seeding on subsequent renders).
187188
useEffect(() => {
188-
if (!isVisible || !tx?.id) return;
189+
if (!isVisible || !tx?.id || !isTransactionModifiable) return;
189190
if (tx.previousGas) return;
190191

191192
const { txParams } = tx;
@@ -214,7 +215,14 @@ export function CancelSpeedupModal({
214215
userFeeLevel: bumpResult.userFeeLevel,
215216
});
216217
}
217-
}, [isVisible, tx?.id, isCancel, gasFeeEstimates, tx]);
218+
}, [
219+
isVisible,
220+
tx?.id,
221+
isCancel,
222+
gasFeeEstimates,
223+
tx,
224+
isTransactionModifiable,
225+
]);
218226

219227
// Dismiss gas modal when parent cancel/speed up modal closes.
220228
useEffect(() => {
@@ -223,15 +231,20 @@ export function CancelSpeedupModal({
223231
}
224232
}, [isVisible]);
225233

234+
// Close the gas modal when the transaction is no longer modifiable.
235+
useEffect(() => {
236+
if (isVisible && gasModalVisible && !isTransactionModifiable) {
237+
setGasModalVisible(false);
238+
}
239+
}, [isVisible, gasModalVisible, isTransactionModifiable]);
240+
226241
const openGasModal = useCallback(() => {
227242
setGasModalVisible(true);
228243
}, []);
229244

230245
const close = useCallback(() => {
231-
bottomSheetRef.current?.onCloseBottomSheet(() => {
232-
onClose();
233-
});
234-
}, [onClose]);
246+
bottomSheetRef.current?.onCloseBottomSheet();
247+
}, []);
235248

236249
const effectiveConfirmDisabled = confirmDisabled || !isInitialGasReady;
237250

@@ -260,56 +273,50 @@ export function CancelSpeedupModal({
260273
];
261274

262275
return (
263-
<>
264-
<Modal
265-
isVisible={isVisible}
266-
animationIn="slideInUp"
267-
animationOut="slideOutDown"
268-
style={modalStyle.bottom}
269-
backdropColor={colors.overlay.default}
270-
backdropOpacity={1}
271-
useNativeDriver
272-
onBackdropPress={onClose}
273-
onBackButtonPress={onClose}
274-
onSwipeComplete={onClose}
275-
swipeDirection="down"
276-
propagateSwipe
276+
<Modal
277+
isVisible={isVisible}
278+
animationIn="slideInUp"
279+
animationOut="slideOutDown"
280+
style={modalStyle.bottom}
281+
backdropColor={colors.overlay.default}
282+
backdropOpacity={1}
283+
useNativeDriver
284+
onBackdropPress={onClose}
285+
onBackButtonPress={onClose}
286+
>
287+
<BottomSheet
288+
ref={bottomSheetRef}
289+
shouldNavigateBack={false}
290+
onClose={onClose}
291+
style={styles.bottomSheetDialogSheet}
277292
>
278-
<BottomSheet
279-
ref={bottomSheetRef}
280-
shouldNavigateBack={false}
281-
style={styles.bottomSheetDialogSheet}
282-
>
283-
<HeaderCompactStandard title={title} onClose={close} />
284-
<Box style={tw.style('px-3')}>
285-
<ScrollView>
286-
<Box gap={4}>
287-
<InfoSection>
288-
<NetworkFeeRow
289-
fiat={networkFeeFiat}
290-
native={networkFeeNative}
291-
symbol={nativeTokenSymbol}
292-
chainId={chainId}
293-
onEditPress={openGasModal}
294-
/>
295-
<SpeedRow transactionId={tx?.id} />
296-
</InfoSection>
297-
<Description text={description} />
298-
</Box>
299-
</ScrollView>
300-
<BottomSheetFooter
301-
buttonsAlignment={ButtonsAlignment.Vertical}
302-
buttonPropsArray={buttons}
303-
style={tw.style('px-0')}
304-
/>
293+
<HeaderCompactStandard title={title} onClose={close} />
294+
<Box style={tw.style('px-3')}>
295+
<Box gap={4}>
296+
<InfoSection>
297+
<NetworkFeeRow
298+
fiat={networkFeeFiat}
299+
native={networkFeeNative}
300+
symbol={nativeTokenSymbol}
301+
chainId={chainId}
302+
onEditPress={isTransactionModifiable ? openGasModal : undefined}
303+
/>
304+
<SpeedRow transactionId={tx?.id} />
305+
</InfoSection>
306+
<Description text={description} />
305307
</Box>
306-
</BottomSheet>
307-
</Modal>
308+
<BottomSheetFooter
309+
buttonsAlignment={ButtonsAlignment.Vertical}
310+
buttonPropsArray={buttons}
311+
style={tw.style('px-0')}
312+
/>
313+
</Box>
314+
</BottomSheet>
308315
{gasModalVisible && tx?.id && (
309316
<GasFeeModalTransactionProvider transactionId={tx.id}>
310317
<GasFeeModal setGasModalVisible={setGasModalVisible} />
311318
</GasFeeModalTransactionProvider>
312319
)}
313-
</>
320+
</Modal>
314321
);
315322
}

‎app/components/Views/confirmations/hooks/gas/useCancelSpeedupGas/types.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ export interface UseCancelSpeedupGasResult {
1717
networkFeeFiat: string | null;
1818
/** Native currency symbol for the chain (e.g. "ETH") */
1919
nativeTokenSymbol: string;
20+
/** Whether the transaction can still be modified (status is unapproved or submitted). */
21+
isTransactionModifiable: boolean;
2022
/** True once previousGas has been persisted and initial gas params applied. */
2123
isInitialGasReady: boolean;
2224
}

‎app/components/Views/confirmations/hooks/gas/useCancelSpeedupGas/useCancelSpeedupGas.test.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import {
22
GasFeeEstimateLevel,
3+
TransactionStatus,
34
UserFeeLevel,
45
type FeeMarketEIP1559Values,
56
type GasPriceValue,
@@ -128,6 +129,7 @@ describe('useCancelSpeedupGas', () => {
128129
expect(result.current.networkFeeFiat).toBeNull();
129130
expect(result.current.nativeTokenSymbol).toBe('ETH');
130131
expect(result.current.isInitialGasReady).toBe(false);
132+
expect(result.current.isTransactionModifiable).toBe(false);
131133
});
132134

133135
it('returns empty result when tx has no txParams', () => {
@@ -347,6 +349,44 @@ describe('useCancelSpeedupGas', () => {
347349
);
348350
expect(result.current.isInitialGasReady).toBe(true);
349351
});
352+
353+
describe('isTransactionModifiable', () => {
354+
it('returns false when txId is null (no tx)', () => {
355+
const { result } = renderHookWithProvider(
356+
() => useCancelSpeedupGas({ txId: null }),
357+
providerState,
358+
);
359+
expect(result.current.isTransactionModifiable).toBe(false);
360+
});
361+
362+
it.each([TransactionStatus.unapproved, TransactionStatus.submitted])(
363+
'returns true when tx status is %s',
364+
(status) => {
365+
const tx = { ...mockTxEip1559, status } as unknown as TransactionMeta;
366+
const { result } = renderHookWithProvider(
367+
() => useCancelSpeedupGas({ txId: 'tx-1' }),
368+
buildStateWithTransaction(tx),
369+
);
370+
expect(result.current.isTransactionModifiable).toBe(true);
371+
},
372+
);
373+
374+
it.each([
375+
TransactionStatus.confirmed,
376+
TransactionStatus.failed,
377+
TransactionStatus.dropped,
378+
TransactionStatus.rejected,
379+
TransactionStatus.approved,
380+
TransactionStatus.signed,
381+
])('returns false when tx status is %s', (status) => {
382+
const tx = { ...mockTxEip1559, status } as unknown as TransactionMeta;
383+
const { result } = renderHookWithProvider(
384+
() => useCancelSpeedupGas({ txId: 'tx-1' }),
385+
buildStateWithTransaction(tx),
386+
);
387+
expect(result.current.isTransactionModifiable).toBe(false);
388+
});
389+
});
350390
});
351391

352392
describe('getBumpParamsForCancelSpeedup', () => {

‎app/components/Views/confirmations/hooks/gas/useCancelSpeedupGas/useCancelSpeedupGas.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {
33
GasFeeEstimateLevel,
44
isEIP1559Transaction,
55
SPEED_UP_RATE,
6+
TransactionStatus,
67
UserFeeLevel,
78
type FeeMarketEIP1559Values,
89
type GasPriceValue,
@@ -32,6 +33,11 @@ import type {
3233

3334
const HEX_ZERO = '0x0';
3435

36+
const MODIFIABLE_STATUSES = new Set([
37+
TransactionStatus.unapproved,
38+
TransactionStatus.submitted,
39+
]);
40+
3541
/** Stub passed to useFeeCalculations when tx is null so the hook is always called unconditionally. */
3642
const STUB_TX = {
3743
txParams: { gas: HEX_ZERO },
@@ -207,6 +213,10 @@ export function useCancelSpeedupGas({
207213

208214
const isInitialGasReady = Boolean(tx?.previousGas);
209215

216+
const isTransactionModifiable = Boolean(
217+
tx?.status && MODIFIABLE_STATUSES.has(tx.status),
218+
);
219+
210220
return useMemo((): UseCancelSpeedupGasResult => {
211221
const empty: UseCancelSpeedupGasResult = {
212222
paramsForController: undefined,
@@ -215,6 +225,7 @@ export function useCancelSpeedupGas({
215225
networkFeeFiat: null,
216226
nativeTokenSymbol,
217227
isInitialGasReady,
228+
isTransactionModifiable,
218229
};
219230

220231
if (!tx?.txParams || !paramsForController) {
@@ -230,6 +241,7 @@ export function useCancelSpeedupGas({
230241
networkFeeFiat: feeCalculations.estimatedFeeFiat,
231242
nativeTokenSymbol,
232243
isInitialGasReady,
244+
isTransactionModifiable,
233245
};
234246
}, [
235247
tx,
@@ -238,5 +250,6 @@ export function useCancelSpeedupGas({
238250
feeCalculations.estimatedFeeFiat,
239251
nativeTokenSymbol,
240252
isInitialGasReady,
253+
isTransactionModifiable,
241254
]);
242255
}

0 commit comments

Comments
 (0)