Skip to content

Commit 9576f7c

Browse files
feat: Fix plan 835 Hid request dialog keep on prompting up during pagination. (#30384)
<!-- 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. --> This PR is relative to solve the [plan 835](MetaMask/accounts-planning#835) This PR including following change: 1. in actions.js file, change `connectHardware` method signature to has extra `loadHid` boolean parameter to see whether we need to load HID request prompt up. 2. change `connect-hardware/index.js` to and pass `loadHid = true` parameter value to above actions. 3. change `connect-hardware/account-list.js` and all pagination feature relative functions to pass `loadHid = false` to disable HID request popup apear. ## **Description** <!-- 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? 5. What is the improvement/solution? --> [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30384?quickstart=1) ## **Related issues** Fixes: MetaMask/accounts-planning#835 ## **Manual testing steps** This test requires a Ledger hardware device with more than 5 accounts: 1. Add hardware wallet 2. Connect to Ledger device 3. Verify 5 Ledger accounts are displayed 4. Click Next 5. Verify the HID popup is not displayed each time the user clicks Next button ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** The following popup is displayed after each "Next" action ![412918093-ba1553a7-e3ba-42f6-bf7d-99b84473fcab](https://github.com/user-attachments/assets/86545f55-be52-4d58-a451-da4a3e010d4e) ### **After** The popup is not displayed repeatedly ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/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-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] 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. --------- Co-authored-by: Michele Esposito <[email protected]>
1 parent d5df7c4 commit 9576f7c

File tree

6 files changed

+387
-15
lines changed

6 files changed

+387
-15
lines changed

ui/pages/create-account/connect-hardware/__snapshots__/index.test.tsx.snap

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// Jest Snapshot v1, https://goo.gl/fbAQLP
22

3-
exports[`ConnectHardwareForm should match snapshot 1`] = `
3+
exports[`ConnectHardwareForm matchs snapshot 1`] = `
44
<div>
55
<div
66
class="mm-box new-external-account-form mm-box--display-flex mm-box--flex-direction-column mm-box--justify-content-center mm-box--align-items-center"

ui/pages/create-account/connect-hardware/account-list.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,14 @@ class AccountList extends Component {
1919
goToNextPage = () => {
2020
// If we have < 5 accounts, it's restricted by BIP-44
2121
if (this.props.accounts.length === 5) {
22-
this.props.getPage(this.props.device, 1, this.props.selectedPath);
22+
this.props.getPage(this.props.device, 1, this.props.selectedPath, false);
2323
} else {
2424
this.props.onAccountRestriction();
2525
}
2626
};
2727

2828
goToPreviousPage = () => {
29-
this.props.getPage(this.props.device, -1, this.props.selectedPath);
29+
this.props.getPage(this.props.device, -1, this.props.selectedPath, false);
3030
};
3131

3232
setPath(pathValue) {

ui/pages/create-account/connect-hardware/index.js

+7-5
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ class ConnectHardwareForm extends Component {
149149
}
150150

151151
// Default values
152-
this.getPage(device, 0, this.props.defaultHdPaths[device]);
152+
this.getPage(device, 0, this.props.defaultHdPaths[device], true);
153153
};
154154

155155
onPathChange = (path) => {
@@ -185,9 +185,9 @@ class ConnectHardwareForm extends Component {
185185
}, SECOND * 5);
186186
}
187187

188-
getPage = (device, page, hdPath) => {
188+
getPage = (device, page, hdPath, loadHid) => {
189189
this.props
190-
.connectHardware(device, page, hdPath, this.context.t)
190+
.connectHardware(device, page, hdPath, loadHid, this.context.t)
191191
.then((accounts) => {
192192
if (accounts.length) {
193193
// If we just loaded the accounts for the first time
@@ -475,8 +475,10 @@ const mapDispatchToProps = (dispatch) => {
475475
setHardwareWalletDefaultHdPath: ({ device, path }) => {
476476
return dispatch(actions.setHardwareWalletDefaultHdPath({ device, path }));
477477
},
478-
connectHardware: (deviceName, page, hdPath, t) => {
479-
return dispatch(actions.connectHardware(deviceName, page, hdPath, t));
478+
connectHardware: (deviceName, page, hdPath, loadHid, t) => {
479+
return dispatch(
480+
actions.connectHardware(deviceName, page, hdPath, loadHid, t),
481+
);
480482
},
481483
checkHardwareStatus: (deviceName, hdPath) => {
482484
return dispatch(actions.checkHardwareStatus(deviceName, hdPath));

ui/pages/create-account/connect-hardware/index.test.tsx

+91-7
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
1-
import configureMockStore from 'redux-mock-store';
21
import { fireEvent, waitFor } from '@testing-library/react';
32
import thunk from 'redux-thunk';
43
import React from 'react';
4+
import configureMockStore from 'redux-mock-store';
5+
56
import { renderWithProvider } from '../../../../test/lib/render-helpers';
67
import {
78
LedgerTransportTypes,
@@ -50,6 +51,7 @@ const mockProps = {
5051
hideAlert: () => jest.fn(),
5152
unlockHardwareWalletAccount: () => jest.fn(),
5253
setHardwareWalletDefaultHdPath: () => jest.fn(),
54+
connectHardware: () => mockConnectHardware,
5355
history: {
5456
push: mockHistoryPush,
5557
},
@@ -95,7 +97,7 @@ const mockState = {
9597
describe('ConnectHardwareForm', () => {
9698
const mockStore = configureMockStore([thunk])(mockState);
9799

98-
it('should match snapshot', () => {
100+
it('matchs snapshot', () => {
99101
const { container } = renderWithProvider(
100102
<ConnectHardwareForm {...mockProps} />,
101103
mockStore,
@@ -104,7 +106,7 @@ describe('ConnectHardwareForm', () => {
104106
expect(container).toMatchSnapshot();
105107
});
106108

107-
it('should close the form when close button is clicked', () => {
109+
it('closes the form when close button is clicked', () => {
108110
const { getByTestId } = renderWithProvider(
109111
<ConnectHardwareForm {...mockProps} />,
110112
mockStore,
@@ -116,7 +118,7 @@ describe('ConnectHardwareForm', () => {
116118
});
117119

118120
describe('U2F Error', () => {
119-
it('should render a U2F error', async () => {
121+
it('renders a U2F error', async () => {
120122
mockConnectHardware.mockRejectedValue(new Error('U2F Error'));
121123
const mockStateWithU2F = Object.assign(mockState, {});
122124
mockStateWithU2F.appState.ledgerTransportType = LedgerTransportTypes.u2f;
@@ -146,7 +148,7 @@ describe('ConnectHardwareForm', () => {
146148
});
147149
});
148150

149-
it('should render a different U2F error for firefox', async () => {
151+
it('renders a different U2F error for firefox', async () => {
150152
jest
151153
.spyOn(window.navigator, 'userAgent', 'get')
152154
.mockReturnValue(
@@ -180,7 +182,7 @@ describe('ConnectHardwareForm', () => {
180182
});
181183

182184
describe('QR Hardware Wallet Steps', () => {
183-
it('should render the QR hardware wallet steps', async () => {
185+
it('renders the QR hardware wallet steps', async () => {
184186
const { getByText, getByLabelText } = renderWithProvider(
185187
<ConnectHardwareForm {...mockProps} />,
186188
mockStore,
@@ -203,7 +205,7 @@ describe('ConnectHardwareForm', () => {
203205
});
204206

205207
describe('Select Hardware', () => {
206-
it('should check link buttons for Ngrave Zero brand', async () => {
208+
it('checks link buttons for Ngrave Zero brand', async () => {
207209
window.open = jest.fn();
208210

209211
const { getByLabelText, getByTestId } = renderWithProvider(
@@ -226,4 +228,86 @@ describe('ConnectHardwareForm', () => {
226228
expect(window.open).toHaveBeenCalled();
227229
});
228230
});
231+
232+
describe('getPage method', () => {
233+
beforeEach(() => {
234+
mockConnectHardware.mockReset();
235+
});
236+
237+
it('calls connectHardware with loadHid=true', async () => {
238+
mockConnectHardware.mockReset();
239+
240+
const mockAccounts = [
241+
{ address: '0xAddress1', balance: null, index: 0 },
242+
{ address: '0xAddress2', balance: null, index: 1 },
243+
];
244+
mockConnectHardware.mockResolvedValue(mockAccounts);
245+
246+
renderWithProvider(<ConnectHardwareForm {...mockProps} />, mockStore);
247+
248+
const hdPath = "m/44'/60'/0'/0";
249+
const deviceName = 'ledger';
250+
const pageIndex = 0;
251+
const loadHidValue = true;
252+
253+
await mockConnectHardware(
254+
deviceName,
255+
pageIndex,
256+
hdPath,
257+
loadHidValue,
258+
jest.fn(),
259+
);
260+
261+
expect(mockConnectHardware).toHaveBeenCalledWith(
262+
deviceName,
263+
pageIndex,
264+
hdPath,
265+
loadHidValue,
266+
expect.any(Function),
267+
);
268+
});
269+
270+
it('calls connectHardware with loadHid=false', async () => {
271+
mockConnectHardware.mockReset();
272+
273+
const mockAccounts = [
274+
{ address: '0xAddress1', balance: null, index: 0 },
275+
{ address: '0xAddress2', balance: null, index: 1 },
276+
];
277+
mockConnectHardware.mockResolvedValue(mockAccounts);
278+
279+
renderWithProvider(<ConnectHardwareForm {...mockProps} />, mockStore);
280+
281+
const hdPath = "m/44'/60'/0'/0";
282+
const deviceName = 'ledger';
283+
const pageIndex = 0;
284+
const loadHidValue = false;
285+
286+
await mockConnectHardware(
287+
deviceName,
288+
pageIndex,
289+
hdPath,
290+
loadHidValue,
291+
jest.fn(),
292+
);
293+
294+
expect(mockConnectHardware).toHaveBeenCalledWith(
295+
deviceName,
296+
pageIndex,
297+
hdPath,
298+
loadHidValue,
299+
expect.any(Function),
300+
);
301+
});
302+
303+
it('handles errors when connectHardware fails', async () => {
304+
const testError = new Error('Test Error');
305+
mockConnectHardware.mockReset();
306+
mockConnectHardware.mockRejectedValue(testError);
307+
308+
renderWithProvider(<ConnectHardwareForm {...mockProps} />, mockStore);
309+
310+
await expect(mockConnectHardware()).rejects.toThrow('Test Error');
311+
});
312+
});
229313
});

0 commit comments

Comments
 (0)