Skip to content

Commit dc9bd4f

Browse files
dawnseeker8cursoragentmetamaskbotNicolasMassart
authored
fix: improve Ledger error handling for disconnect, retry, and pagination cp-7.77.0 (#28515)
## **Description** Fixes multiple issues with Ledger hardware wallet error handling that caused confusing error dialogs when the device was disconnected, the Ethereum app was closed, or blind signing was disabled: 1. **Stale BLE transport cleanup** — `#closeTransport` now forces BLE disconnection even when the transport object is already null but a device ID remains, preventing the OS from keeping a stale connection that blocks reconnection. 2. **Broader transient error detection** — `#isTransientBleError` now falls back to message-based matching (e.g. "disconnected", "bluetooth", "gatt") for BLE errors that use generic `Error` names after a device power-cycle. 3. **Wrong-app command failure cleanup** — `#handleWrongApp` now closes the transport when `openEthereumAppOnLedger` or `closeRunningAppOnLedger` fails, preventing stale connections. 4. **Pagination device readiness** — `nextPage`/`prevPage` in `LedgerSelectAccount` now call `ensureDeviceReady` before fetching accounts, and only dismiss the blocking modal if it was actually shown (via a `modalShown` flag). 5. **Disconnect error classification** — `getLedgerAccountsByOperation` now detects disconnect errors and throws a user-friendly "device got disconnected" message instead of a generic error. 6. **Gitignore updates** — Added `.cursor/hooks/state/` to `.gitignore` to prevent Cursor IDE state files from being committed. ## **Changelog** CHANGELOG entry: Fixed Ledger hardware wallet error handling to properly recover from disconnects, app switching failures, and pagination errors ## **Related issues** Fixes: #28272 ## **Manual testing steps** ```gherkin Feature: Ledger error handling on disconnect Scenario: User disconnects Ledger during account pagination Given the user has connected a Ledger device via Bluetooth And the Ledger Select Account screen is displayed When the user disconnects the Ledger device And the user taps Next Page Then the app should prompt the user to reconnect instead of showing a generic error Scenario: User switches apps on Ledger during operation Given the user has connected a Ledger device with the Ethereum app open When the user switches to a different app on the Ledger And an operation is attempted Then the app should detect the transient BLE error and retry the connection Scenario: Ledger is powered off during account fetch Given the user has connected a Ledger device via Bluetooth And the Ledger Select Account screen is displayed When the user powers off the Ledger device And the user taps Next Page or Previous Page Then the app should show a disconnect-specific error message And the blocking modal should not be incorrectly dismissed ``` ## **Screenshots/Recordings** ### **Before** N/A ### **After** N/A ## **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 touch Ledger BLE transport cleanup/retry logic and account-pagination readiness gating, which can affect hardware-wallet connection stability and user flows. Risk is mitigated by expanded unit test coverage across adapter, error parsing, and UI pagination cases. > > **Overview** > Improves Ledger Bluetooth robustness by forcing BLE cleanup even when the transport object is already cleared, and by expanding transient disconnect detection to include message-based matches; it also closes the transport when app-switch commands fail to avoid stale connections. > > Updates `LedgerSelectAccount` pagination (`nextPage`/`prevPage`) to call `ensureDeviceReady` before fetching pages and to only dismiss the blocking modal if it was shown, preventing unnecessary modal flicker and avoiding pagination attempts when the device isn’t ready. > > Enhances Ledger error classification by treating disconnect-like failures in `getLedgerAccountsByOperation` as a user-facing “disconnected” error, and broadens `parseErrorByType` to handle non-`Error` objects with Ledger-shaped `name`/`statusCode`; adds/updates unit tests across these scenarios. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 5e139f5. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Xiaoming Wang <dawnseeker8@users.noreply.github.com> Co-authored-by: metamaskbot <metamaskbot@users.noreply.github.com> Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>
1 parent 0c60aeb commit dc9bd4f

10 files changed

Lines changed: 489 additions & 25 deletions

File tree

‎app/components/Views/LedgerSelectAccount/index.test.tsx‎

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -818,5 +818,129 @@ describe('LedgerSelectAccount', () => {
818818
expect(queryByText('Network error')).toBeOnTheScreen();
819819
});
820820
});
821+
822+
it('calls ensureDeviceReady before nextPage pagination', async () => {
823+
const { getByTestId } = await renderAndWaitForAccounts();
824+
825+
mockEnsureDeviceReady.mockClear();
826+
mockGetLedgerAccountsByOperation.mockClear();
827+
mockGetLedgerAccountsByOperation.mockResolvedValue(mockAccounts);
828+
829+
await act(async () => {
830+
fireEvent.press(getByTestId(AccountSelectorSelectorsIDs.NEXT_BUTTON));
831+
});
832+
833+
await waitFor(() => {
834+
expect(mockEnsureDeviceReady).toHaveBeenCalledWith('test-device-id');
835+
});
836+
});
837+
838+
it('calls ensureDeviceReady before prevPage pagination', async () => {
839+
const { getByTestId } = await renderAndWaitForAccounts();
840+
841+
mockEnsureDeviceReady.mockClear();
842+
mockGetLedgerAccountsByOperation.mockClear();
843+
mockGetLedgerAccountsByOperation.mockResolvedValue(mockAccounts);
844+
845+
await act(async () => {
846+
fireEvent.press(
847+
getByTestId(AccountSelectorSelectorsIDs.PREVIOUS_BUTTON),
848+
);
849+
});
850+
851+
await waitFor(() => {
852+
expect(mockEnsureDeviceReady).toHaveBeenCalledWith('test-device-id');
853+
});
854+
});
855+
856+
it('skips pagination when ensureDeviceReady returns false on nextPage', async () => {
857+
const { getByTestId } = await renderAndWaitForAccounts();
858+
859+
mockEnsureDeviceReady.mockResolvedValue(false);
860+
mockGetLedgerAccountsByOperation.mockClear();
861+
862+
await act(async () => {
863+
fireEvent.press(getByTestId(AccountSelectorSelectorsIDs.NEXT_BUTTON));
864+
});
865+
866+
await waitFor(() => {
867+
expect(mockEnsureDeviceReady).toHaveBeenCalled();
868+
});
869+
870+
expect(mockGetLedgerAccountsByOperation).not.toHaveBeenCalled();
871+
});
872+
873+
it('skips pagination when ensureDeviceReady returns false on prevPage', async () => {
874+
const { getByTestId } = await renderAndWaitForAccounts();
875+
876+
mockEnsureDeviceReady.mockResolvedValue(false);
877+
mockGetLedgerAccountsByOperation.mockClear();
878+
879+
await act(async () => {
880+
fireEvent.press(
881+
getByTestId(AccountSelectorSelectorsIDs.PREVIOUS_BUTTON),
882+
);
883+
});
884+
885+
await waitFor(() => {
886+
expect(mockEnsureDeviceReady).toHaveBeenCalled();
887+
});
888+
889+
expect(mockGetLedgerAccountsByOperation).not.toHaveBeenCalled();
890+
});
891+
892+
it('shows inline error when ensureDeviceReady throws during nextPage without blocking modal', async () => {
893+
mockEnsureDeviceReady
894+
.mockResolvedValueOnce(true)
895+
.mockRejectedValueOnce(new Error('Device readiness check failed'));
896+
897+
const { getByTestId, queryByText } = await renderAndWaitForAccounts();
898+
899+
await act(async () => {
900+
fireEvent.press(getByTestId(AccountSelectorSelectorsIDs.NEXT_BUTTON));
901+
});
902+
903+
await waitFor(() => {
904+
expect(queryByText('Device readiness check failed')).toBeOnTheScreen();
905+
});
906+
expect(queryByText('Please wait')).not.toBeOnTheScreen();
907+
});
908+
909+
it('shows inline error when ensureDeviceReady throws during prevPage without blocking modal', async () => {
910+
mockEnsureDeviceReady
911+
.mockResolvedValueOnce(true)
912+
.mockRejectedValueOnce(new Error('Bluetooth adapter unavailable'));
913+
914+
const { getByTestId, queryByText } = await renderAndWaitForAccounts();
915+
916+
await act(async () => {
917+
fireEvent.press(
918+
getByTestId(AccountSelectorSelectorsIDs.PREVIOUS_BUTTON),
919+
);
920+
});
921+
922+
await waitFor(() => {
923+
expect(queryByText('Bluetooth adapter unavailable')).toBeOnTheScreen();
924+
});
925+
expect(queryByText('Please wait')).not.toBeOnTheScreen();
926+
});
927+
928+
it('does not show blocking modal when ensureDeviceReady returns false on nextPage', async () => {
929+
mockEnsureDeviceReady
930+
.mockResolvedValueOnce(true)
931+
.mockResolvedValueOnce(false);
932+
933+
const { getByTestId, queryByText } = await renderAndWaitForAccounts();
934+
935+
await act(async () => {
936+
fireEvent.press(getByTestId(AccountSelectorSelectorsIDs.NEXT_BUTTON));
937+
});
938+
939+
await waitFor(() => {
940+
expect(mockEnsureDeviceReady).toHaveBeenCalled();
941+
});
942+
943+
expect(queryByText('Please wait')).not.toBeOnTheScreen();
944+
});
821945
});
822946
});

‎app/components/Views/LedgerSelectAccount/index.tsx‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -222,32 +222,46 @@ const LedgerSelectAccount = () => {
222222
}, [selectedOption]);
223223

224224
const nextPage = useCallback(async () => {
225-
showLoadingModal();
225+
let modalShown = false;
226226
try {
227+
const isReady = await ensureDeviceReady(deviceId);
228+
if (!isReady) return;
229+
230+
showLoadingModal();
231+
modalShown = true;
227232
const _accounts = await getLedgerAccountsByOperation(
228233
PAGINATION_OPERATIONS.GET_NEXT_PAGE,
229234
);
230235
setAccounts(_accounts);
231236
} catch (e) {
232237
setErrorMsg((e as Error).message);
233238
} finally {
234-
setBlockingModalVisible(false);
239+
if (modalShown) {
240+
setBlockingModalVisible(false);
241+
}
235242
}
236-
}, []);
243+
}, [ensureDeviceReady, deviceId]);
237244

238245
const prevPage = useCallback(async () => {
239-
showLoadingModal();
246+
let modalShown = false;
240247
try {
248+
const isReady = await ensureDeviceReady(deviceId);
249+
if (!isReady) return;
250+
251+
showLoadingModal();
252+
modalShown = true;
241253
const _accounts = await getLedgerAccountsByOperation(
242254
PAGINATION_OPERATIONS.GET_PREVIOUS_PAGE,
243255
);
244256
setAccounts(_accounts);
245257
} catch (e) {
246258
setErrorMsg((e as Error).message);
247259
} finally {
248-
setBlockingModalVisible(false);
260+
if (modalShown) {
261+
setBlockingModalVisible(false);
262+
}
249263
}
250-
}, []);
264+
}, [ensureDeviceReady, deviceId]);
251265

252266
const updateNewLegacyAccountsLabel = useCallback(async () => {
253267
if (LEDGER_LEGACY_PATH === (await getHDPath())) {

‎app/core/HardwareWallet/adapters/LedgerBluetoothAdapter.test.ts‎

Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,27 @@ describe('LedgerBluetoothAdapter', () => {
271271
});
272272

273273
describe('disconnect', () => {
274+
it('forces disconnectDevice when BLE disconnect already cleared transport but deviceId remains', async () => {
275+
await adapter.connect('device-123');
276+
mockedTransportBLE.disconnectDevice.mockClear();
277+
278+
const disconnectHandler = mockTransportInstance.on.mock.calls.find(
279+
(call: [string, () => void]) => call[0] === 'disconnect',
280+
)?.[1];
281+
expect(disconnectHandler).toBeDefined();
282+
disconnectHandler?.();
283+
284+
expect(adapter.isConnected()).toBe(false);
285+
expect(adapter.getConnectedDeviceId()).toBe('device-123');
286+
287+
await adapter.disconnect();
288+
289+
expect(mockedTransportBLE.disconnectDevice).toHaveBeenCalledWith(
290+
'device-123',
291+
);
292+
expect(adapter.getConnectedDeviceId()).toBeNull();
293+
});
294+
274295
it('closes transport and resets state', async () => {
275296
await adapter.connect('device-123');
276297
await adapter.disconnect();
@@ -625,6 +646,21 @@ describe('LedgerBluetoothAdapter', () => {
625646
},
626647
);
627648

649+
it('retries when address verification fails with message-based transient GATT error', async () => {
650+
jest.mocked(connectLedgerHardware).mockResolvedValue('Ethereum');
651+
const gattError = new Error('GATT server disconnected');
652+
gattError.name = 'Error';
653+
mockGetAddress
654+
.mockRejectedValueOnce(gattError)
655+
.mockResolvedValueOnce({ address: '0x1234' });
656+
657+
const result = await adapter.ensureDeviceReady('device-123');
658+
659+
expect(result).toBe(true);
660+
expect(mockGetAddress).toHaveBeenCalledTimes(2);
661+
expect(connectLedgerHardware).toHaveBeenCalledTimes(2);
662+
});
663+
628664
it('emits DeviceLocked when getAddress fails with Locked device message', async () => {
629665
jest.mocked(connectLedgerHardware).mockResolvedValue('Ethereum');
630666
mockGetAddress.mockRejectedValueOnce(
@@ -687,6 +723,160 @@ describe('LedgerBluetoothAdapter', () => {
687723

688724
jest.useRealTimers();
689725
});
726+
727+
it('retries when error message contains "disconnected" even with generic Error name', async () => {
728+
const genericBleError = new Error('Device disconnected during operation');
729+
genericBleError.name = 'Error';
730+
jest
731+
.mocked(connectLedgerHardware)
732+
.mockRejectedValueOnce(genericBleError)
733+
.mockResolvedValueOnce('Ethereum');
734+
mockGetAddress.mockResolvedValue({ address: '0x1234' });
735+
736+
const result = await adapter.ensureDeviceReady('device-123');
737+
738+
expect(result).toBe(true);
739+
expect(connectLedgerHardware).toHaveBeenCalledTimes(2);
740+
});
741+
742+
it('retries when error message contains "bluetooth connection" even with generic Error name', async () => {
743+
const genericBleError = new Error('Bluetooth connection failed');
744+
genericBleError.name = 'Error';
745+
jest
746+
.mocked(connectLedgerHardware)
747+
.mockRejectedValueOnce(genericBleError)
748+
.mockResolvedValueOnce('Ethereum');
749+
mockGetAddress.mockResolvedValue({ address: '0x1234' });
750+
751+
const result = await adapter.ensureDeviceReady('device-123');
752+
753+
expect(result).toBe(true);
754+
expect(connectLedgerHardware).toHaveBeenCalledTimes(2);
755+
});
756+
757+
it('retries when error message contains "bluetooth transfer" even with generic Error name', async () => {
758+
const genericBleError = new Error('Bluetooth transfer interrupted');
759+
genericBleError.name = 'Error';
760+
jest
761+
.mocked(connectLedgerHardware)
762+
.mockRejectedValueOnce(genericBleError)
763+
.mockResolvedValueOnce('Ethereum');
764+
mockGetAddress.mockResolvedValue({ address: '0x1234' });
765+
766+
const result = await adapter.ensureDeviceReady('device-123');
767+
768+
expect(result).toBe(true);
769+
expect(connectLedgerHardware).toHaveBeenCalledTimes(2);
770+
});
771+
772+
it('retries when error message contains "connection lost" even with generic Error name', async () => {
773+
const genericBleError = new Error('The connection lost unexpectedly');
774+
genericBleError.name = 'Error';
775+
jest
776+
.mocked(connectLedgerHardware)
777+
.mockRejectedValueOnce(genericBleError)
778+
.mockResolvedValueOnce('Ethereum');
779+
mockGetAddress.mockResolvedValue({ address: '0x1234' });
780+
781+
const result = await adapter.ensureDeviceReady('device-123');
782+
783+
expect(result).toBe(true);
784+
expect(connectLedgerHardware).toHaveBeenCalledTimes(2);
785+
});
786+
787+
it('retries when error message contains "gatt" even with generic Error name', async () => {
788+
const genericBleError = new Error('GATT operation failed');
789+
genericBleError.name = 'Error';
790+
jest
791+
.mocked(connectLedgerHardware)
792+
.mockRejectedValueOnce(genericBleError)
793+
.mockResolvedValueOnce('Ethereum');
794+
mockGetAddress.mockResolvedValue({ address: '0x1234' });
795+
796+
const result = await adapter.ensureDeviceReady('device-123');
797+
798+
expect(result).toBe(true);
799+
expect(connectLedgerHardware).toHaveBeenCalledTimes(2);
800+
});
801+
802+
it('retries when error message contains "ble error" even with generic Error name', async () => {
803+
const genericBleError = new Error('Native BLE error on channel');
804+
genericBleError.name = 'Error';
805+
jest
806+
.mocked(connectLedgerHardware)
807+
.mockRejectedValueOnce(genericBleError)
808+
.mockResolvedValueOnce('Ethereum');
809+
mockGetAddress.mockResolvedValue({ address: '0x1234' });
810+
811+
const result = await adapter.ensureDeviceReady('device-123');
812+
813+
expect(result).toBe(true);
814+
expect(connectLedgerHardware).toHaveBeenCalledTimes(2);
815+
});
816+
817+
it.each([
818+
'Bluetooth is off',
819+
'Bluetooth not supported',
820+
'Not authorized to use Bluetooth',
821+
'Bluetooth permission denied',
822+
'Bluetooth scan failed',
823+
])(
824+
'does not retry for non-transient bluetooth error: "%s"',
825+
async (errorMessage) => {
826+
const nonTransientError = new Error(errorMessage);
827+
nonTransientError.name = 'Error';
828+
jest.mocked(connectLedgerHardware).mockRejectedValue(nonTransientError);
829+
830+
await expect(adapter.ensureDeviceReady('device-123')).rejects.toThrow();
831+
expect(connectLedgerHardware).toHaveBeenCalledTimes(1);
832+
},
833+
);
834+
835+
it('forces BLE cleanup when transport is null but deviceId exists during retry', async () => {
836+
const disconnectError = new Error('Disconnected');
837+
disconnectError.name = 'DisconnectedDevice';
838+
jest
839+
.mocked(connectLedgerHardware)
840+
.mockRejectedValueOnce(disconnectError)
841+
.mockResolvedValueOnce('Ethereum');
842+
mockGetAddress.mockResolvedValue({ address: '0x1234' });
843+
844+
await adapter.ensureDeviceReady('device-123');
845+
846+
expect(mockedTransportBLE.disconnectDevice).toHaveBeenCalled();
847+
});
848+
849+
it('closes transport when openEthereumAppOnLedger fails', async () => {
850+
jest.mocked(connectLedgerHardware).mockResolvedValue('BOLOS');
851+
const { openEthereumAppOnLedger } = jest.requireMock(
852+
'../../Ledger/Ledger',
853+
) as { openEthereumAppOnLedger: jest.Mock };
854+
openEthereumAppOnLedger.mockRejectedValueOnce(
855+
new Error('User cancelled'),
856+
);
857+
858+
await adapter.ensureDeviceReady('device-123');
859+
860+
expect(mockedTransportBLE.disconnectDevice).toHaveBeenCalledWith(
861+
'device-123',
862+
);
863+
});
864+
865+
it('closes transport when closeRunningAppOnLedger fails', async () => {
866+
jest.mocked(connectLedgerHardware).mockResolvedValue('Bitcoin');
867+
const { closeRunningAppOnLedger } = jest.requireMock(
868+
'../../Ledger/Ledger',
869+
) as { closeRunningAppOnLedger: jest.Mock };
870+
closeRunningAppOnLedger.mockRejectedValueOnce(
871+
new Error('User cancelled'),
872+
);
873+
874+
await adapter.ensureDeviceReady('device-123');
875+
876+
expect(mockedTransportBLE.disconnectDevice).toHaveBeenCalledWith(
877+
'device-123',
878+
);
879+
});
690880
});
691881

692882
describe('reset', () => {

0 commit comments

Comments
 (0)