Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -451,13 +451,14 @@ describe('LedgerBluetoothAdapter', () => {
);
});

it('returns false when no transport after connect', async () => {
it('throws DisconnectedDevice when no transport after connect', async () => {
mockedTransportBLE.open.mockResolvedValueOnce(
null as unknown as TransportBLE,
);

const result = await adapter.ensureDeviceReady('device-123');
expect(result).toBe(false);
await expect(adapter.ensureDeviceReady('device-123')).rejects.toMatchObject(
{ name: 'DisconnectedDevice' },
);
});

it('retries on disconnect during check and eventually succeeds', async () => {
Expand Down
14 changes: 12 additions & 2 deletions app/core/HardwareWallet/adapters/LedgerBluetoothAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,12 @@ export class LedgerBluetoothAdapter implements HardwareWalletAdapter {

if (!this.#transport) {
DevLogger.log('[LedgerBluetoothAdapter] No transport after connect');
return false;
// Transport was cleared by a disconnect event immediately after
// connect(). Throw a named disconnect error so the retry loop can
// treat it as transient and retry rather than silently returning false.
const err = new Error('Transport disconnected after connect');
err.name = 'DisconnectedDevice';
throw err;
}

try {
Expand Down Expand Up @@ -452,7 +457,12 @@ export class LedgerBluetoothAdapter implements HardwareWalletAdapter {

try {
if (!this.#transport) {
throw new Error('Transport not available');
// Transport was cleared by a disconnect event between connect() and
// this verification step. Throw a named disconnect error so the
// ensureDeviceReady retry loop can treat it as transient and retry.
const err = new Error('Transport disconnected during verification');
err.name = 'DisconnectedDeviceDuringOperation';
throw err;
}
const eth = new Eth(this.#transport);
await this.#withTimeout(
Expand Down
76 changes: 76 additions & 0 deletions app/core/HardwareWallet/hooks/useDeviceConnectionFlow.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,4 +116,80 @@ describe('useDeviceConnectionFlow', () => {
});
});
});

describe('retryEnsureDeviceReady with stored device ID', () => {
it('uses pendingTargetDeviceId when state.deviceId is null', async () => {
const mockEnsureDeviceReady = jest.fn().mockResolvedValue(true);
const mockAdapter = {
walletType: HardwareWalletType.Ledger,
resetFlowState: jest.fn(),
ensurePermissions: jest.fn().mockResolvedValue(true),
ensureDeviceReady: mockEnsureDeviceReady,
markFlowComplete: jest.fn(),
isFlowComplete: jest.fn().mockReturnValue(false),
requiresDeviceDiscovery: false,
};
const refs = createMockRefs();
refs.adapterRef.current = mockAdapter as unknown as Parameters<
typeof useDeviceConnectionFlow
>[0]['refs']['adapterRef']['current'];

const options = createDefaultOptions({
refs,
// state.deviceId is null – retry must fall back to stored target ID
deviceId: null,
checkTransportEnabledOrShowError: jest.fn().mockResolvedValue(false),
});

const { result } = renderHook(() => useDeviceConnectionFlow(options));

// Simulate ensureDeviceReady being called with a device ID (stores it)
await act(async () => {
result.current.ensureDeviceReady('device-from-nav-params');
// Don't await the blocking promise; just let the inner IIFE start
await Promise.resolve();
});

// Now retry – state.deviceId is still null but stored ID should be used
await act(async () => {
await result.current.retryEnsureDeviceReady();
});

expect(mockEnsureDeviceReady).toHaveBeenCalledWith(
'device-from-nav-params',
);
expect(options.updateConnectionState).toHaveBeenCalledWith(
expect.objectContaining({ status: ConnectionStatus.Connecting }),
);
});

it('falls through to scanning when both state.deviceId and stored ID are null', async () => {
const mockAdapter = {
walletType: HardwareWalletType.Ledger,
resetFlowState: jest.fn(),
ensurePermissions: jest.fn().mockResolvedValue(true),
requiresDeviceDiscovery: true,
};
const refs = createMockRefs();
refs.adapterRef.current = mockAdapter as unknown as Parameters<
typeof useDeviceConnectionFlow
>[0]['refs']['adapterRef']['current'];

const options = createDefaultOptions({
refs,
deviceId: null,
checkTransportEnabledOrShowError: jest.fn().mockResolvedValue(false),
});

const { result } = renderHook(() => useDeviceConnectionFlow(options));

await act(async () => {
await result.current.retryEnsureDeviceReady();
});

expect(options.updateConnectionState).toHaveBeenCalledWith(
expect.objectContaining({ status: ConnectionStatus.Scanning }),
);
});
});
});
23 changes: 21 additions & 2 deletions app/core/HardwareWallet/hooks/useDeviceConnectionFlow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,15 @@ export const useDeviceConnectionFlow = ({

const connectionSuccessCallbackRef = useRef<(() => void) | null>(null);

/**
* Stores the deviceId passed to the most recent `ensureDeviceReady` call.
* Used by `retryEnsureDeviceReady` to connect to the correct device even
* when `state.deviceId` has not been populated yet (e.g. the initial BLE
* open failed before `DeviceEvent.Connected` was emitted, or on a fresh
* app launch where state was never hydrated).
*/
const pendingTargetDeviceIdRef = useRef<string | null>(null);

/**
* Resolve an existing adapter or create a new one if the wallet type
* doesn't match. Named replacement for the inline IIFE that was previously
Expand Down Expand Up @@ -208,6 +217,10 @@ export const useDeviceConnectionFlow = ({

if (!targetDeviceId) {
setters.setDeviceId(null);
} else {
// Remember the device ID so retryEnsureDeviceReady can use it even
// when state.deviceId has not been populated yet.
pendingTargetDeviceIdRef.current = targetDeviceId;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Stale pendingTargetDeviceIdRef not cleared for no-device flows

Medium Severity

When ensureDeviceReady is called without a targetDeviceId (falsy), pendingTargetDeviceIdRef is not cleared. If a previous call stored a device ID in the ref, a subsequent retryEnsureDeviceReady will use the stale value via deviceId ?? pendingTargetDeviceIdRef.current instead of falling through to scanning mode. The if (!targetDeviceId) branch needs to set pendingTargetDeviceIdRef.current = null alongside setDeviceId(null).

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit aa97022. Configure here.

}

const adapter = resolveOrCreateAdapter(targetType);
Expand Down Expand Up @@ -276,10 +289,15 @@ export const useDeviceConnectionFlow = ({
return;
}

if (deviceId && adapter) {
// Prefer the runtime state deviceId; fall back to the id stored when
// ensureDeviceReady was last called (covers the case where the initial
// BLE open failed before DeviceEvent.Connected could set state.deviceId).
const retryDeviceId = deviceId ?? pendingTargetDeviceIdRef.current;

if (retryDeviceId && adapter) {
updateConnectionState({ status: ConnectionStatus.Connecting });
try {
await tryEnsureReady(adapter, deviceId);
await tryEnsureReady(adapter, retryDeviceId);
} catch (error) {
handleError(error);
}
Expand All @@ -300,6 +318,7 @@ export const useDeviceConnectionFlow = ({
pendingReadyResolveRef.current(false);
pendingReadyResolveRef.current = null;
}
pendingTargetDeviceIdRef.current = null;
setters.setTargetWalletType(null);
updateConnectionState({ status: ConnectionStatus.Disconnected });
}, [setters, updateConnectionState]);
Expand Down
Loading